On Fri, 16 Oct 2020 18:09:48 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Sun, Oct 11, 2020 at 9:24 PM Lino Sanfilippo <LinoSanfilippo@xxxxxx> wrote:
> >
> > In function map_array_register() properly rewind in case of error.
> > Furthermore remove the now superfluous initialization of "ret" with 0.
>
> > int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> > {
> > - int i = 0, ret = 0;
> > + int i = 0, ret;
> > struct iio_map_internal *mapi;
> >
> > if (maps == NULL)
> > @@ -44,7 +44,18 @@ int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> > list_add_tail(&mapi->l, &iio_map_list);
> > i++;
> > }
> > + mutex_unlock(&iio_map_list_lock);
> > +
> > + return 0;
> > +
> > error_ret:
>
> Wait a bit.
> First of all we linked all successfully added items to the list.
> From this we have two ways to go:
> - leave with as many maps as we registered
> - clean up and bail out
>
> I dunno which one would play better in IIO, but you seem to go with
> the latter one.
Better to cleanup and bail out I think. It's fairly unlikely
a consumer is going to be ready to cope with getting a partial
set of the channels it's expecting.
>
> > + /* undo */
> > + while (i--) {
> > + mapi = list_last_entry(&iio_map_list, struct iio_map_internal,
> > + l);
> > + list_del(&mapi->l);
> > + kfree(mapi);
> > + }
>
> We have iio_map_array_unregister(). Why not use it?
>
> > mutex_unlock(&iio_map_list_lock);
>
> I would rather drop a label with replacement goto -> break inside the
> loop and call the following
I argued for the goto, but it is indeed less obviously the right
thing to do once we are using iio_map_array_unregister.
>
>
> mutex_unlock(&iio_map_list_lock);
> if (ret)
> iio_map_array_unregister();
> return ret;
>
> Sounds like only a few LOCs are needed.
>
