[dm-devel] [PATCH 4/6] libmultipath: check blist before calling MALLOC in alloc_ble_device func

[Benjamin Marzinski]

On Mon, Aug 17, 2020 at 10:51:49AM +0200, Martin Wilck wrote:
> On Sun, 2020-08-16 at 09:45 +0800, Zhiqiang Liu wrote:
> > In alloc_ble_device func, ble is firstly allocated by calling MALLOC,
> > and then input blist is checked whether it is valid. If blist is not
> > valid, ble will be freed without using.
> > 
> > Here, we should check blist firstly.
> > 
> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26 at huawei.com>
> > Signed-off-by: lixiaokeng <lixiaokeng at huawei.com>
> > ---
> >  libmultipath/blacklist.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> This patch isn't wrong, but it fixes code which isn't buggy. It's
> rather a style thing, an optimization for an extremely unlikely error
> case. I agree with you in the sense that I prefer the "new" style over
> the old (I generally dislike expressions that can fail, like malloc()
> calls, being used as variable initializers), but I'm not sure if we
> should start applying patches for cases like this. So far we've been
> rather conservative with "style" patches, because they tend to make it
> unnecessarily hard to track code history.
> 
> Ben, Christophe, what's your take on this matter?

While I'm not really a fan of whitespace tweaking patches, I'm fine with
this. All things being equal, I really do prefer it when functions check
their arguments first, instead of doing possibly unnecessary work.

Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>
 
> Regards,
> Martin
> 
> 
> > 
> > diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> > index db58ccc..bedcc7e 100644
> > --- a/libmultipath/blacklist.c
> > +++ b/libmultipath/blacklist.c
> > @@ -66,12 +66,16 @@ out:
> > 
> >  int alloc_ble_device(vector blist)
> >  {
> > -	struct blentry_device * ble = MALLOC(sizeof(struct
> > blentry_device));
> > +	struct blentry_device *ble;
> > 
> > +	if (!blist)
> > +		return 1;
> > +
> > +	ble = MALLOC(sizeof(struct blentry_device));
> >  	if (!ble)
> >  		return 1;
> > 
> > -	if (!blist || !vector_alloc_slot(blist)) {
> > +	if (!vector_alloc_slot(blist)) {
> >  		FREE(ble);
> >  		return 1;
> >  	}
>