Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 16 Oct 2020 18:09:48 +0300 | Subject | Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error |
| |
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.
> + /* 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
mutex_unlock(&iio_map_list_lock); if (ret) iio_map_array_unregister(); return ret;
Sounds like only a few LOCs are needed.
-- With Best Regards, Andy Shevchenko
| |