Messages in this thread | | | Date | Sun, 18 Oct 2020 10:56:31 +0100 | From | Jonathan Cameron <> | Subject | Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error |
| |
On Fri, 16 Oct 2020 18:09:48 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Sun, Oct 11, 2020 at 9:24 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> 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. >
| |