On Sun, 11 Oct 2020 14:41:20 +0200 Lino Sanfilippo <[email protected]> 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 <[email protected]> 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 >
