Re: [PATCH] iio:core: In map_array_register() cleanup in case of error

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 11 Oct 2020 14:41:20 +0200
Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:

> In function map_array_register() properly rewind in case of error.
> Furthermore save an extra label by using a break instead of goto to leave
> the concerning loop.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Good spot. I'd rather we went with a different code flow though.
See below.

Thanks,

Jonathan


> ---
>  drivers/iio/inkern.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 5a8351c..0735cc4 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -28,6 +28,7 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>  {
>  	int i = 0, ret = 0;
>  	struct iio_map_internal *mapi;
> +	struct list_head *pos, *tmp;
> 
>  	if (maps == NULL)
>  		return 0;
> @@ -37,14 +38,22 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>  		mapi = kzalloc(sizeof(*mapi), GFP_KERNEL);
>  		if (mapi == NULL) {
>  			ret = -ENOMEM;
> -			goto error_ret;
> +			break;
>  		}
>  		mapi->map = &maps[i];
>  		mapi->indio_dev = indio_dev;
>  		list_add_tail(&mapi->l, &iio_map_list);
>  		i++;
>  	}
> -error_ret:
> +
Please do this as a a separate error path rather than having
it as non trivial unwinding in the main code flow.

i.e. goto error_ret, but handle that path separately after
the return below.

> +	if (ret) { /* undo */
> +		while (i--) {
> +			mapi = list_last_entry(&iio_map_list,
> +					       struct iio_map_internal, l);
> +			list_del(&mapi->l);
> +			kfree(mapi);
> +		}
> +	}
>  	mutex_unlock(&iio_map_list_lock);
> 
>  	return ret;
> --
> 2.7.4
>