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

[Martin Wilck]

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?

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;
>  	}