| Message ID | 1602440546-2376-1-git-send-email-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio:core: In map_array_register() cleanup in case of error | expand |
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.
Hi Andy, Thanks for you suggestion. I agree that it is nicer to reuse the cleanup in iio_map_array_unregister() than to reimplement it. However I would like to avoid dropping and regaining the mutex in case of error. What about the attached approach: Introduce an unlocked version of iio_map_array_unregister() and then use this one to unwind in error case. Thus we only need to add two lines of code, so that Jonathan may be ok with adding it to the main code flow... Regards, Lino
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. >
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c index 5a8351c..9dedc30 100644 --- a/drivers/iio/inkern.c +++ b/drivers/iio/inkern.c @@ -26,7 +26,7 @@ static DEFINE_MUTEX(iio_map_list_lock); 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: + /* 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;
In function map_array_register() properly rewind in case of error. Furthermore remove the now superfluous initialization of "ret" with 0. Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> --- drivers/iio/inkern.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) -- 2.7.4