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@xxxxxxxxxx>
> > Signed-off-by: lixiaokeng <lixiaokeng@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> 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;
> > }
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
