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 wrote:
> On Sun, Oct 11, 2020 at 9:24 PM Lino Sanfilippo 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(>l, _map_list);
> > i++;
> > }
> > + mutex_unlock(_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(_map_list, struct
> > iio_map_internal,
> > + l);
> > + list_del(>l);
> > + kfree(mapi);
> > + }
>
> We have iio_map_array_unregister(). Why not use it?
>
> > mutex_unlock(_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(_map_list_lock);
> if (ret)
> iio_map_array_unregister();
> return ret;
>
> Sounds like only a few LOCs are needed.
>
Re: [PATCH v2] iio:core: In map_array_register() cleanup in case of error
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
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 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(>l, _map_list);
> i++;
> }
> + mutex_unlock(_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(_map_list, struct iio_map_internal,
> + l);
> + list_del(>l);
> + kfree(mapi);
> + }
We have iio_map_array_unregister(). Why not use it?
> mutex_unlock(_map_list_lock);
I would rather drop a label with replacement goto -> break inside the
loop and call the following
mutex_unlock(_map_list_lock);
if (ret)
iio_map_array_unregister();
return ret;
Sounds like only a few LOCs are needed.
--
With Best Regards,
Andy Shevchenko
