Re: [PATCH v3 3/4] iio: buffer: iio: core: move to the cleanup.h magic

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sat, 16 Mar 2024 21:38:04 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Thu, Feb 29, 2024 at 04:10:27PM +0100, Nuno Sa kirjoitti:
> > Use the new cleanup magic for handling mutexes in IIO. This allows us to
> > greatly simplify some code paths.  
> 
> ...
> 
> >  	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
> >  	if (ret < 0)
> > -		goto error_ret;
> > -	if (!state && ret) {
> > -		ret = iio_scan_mask_clear(buffer, this_attr->address);
> > -		if (ret)
> > -			goto error_ret;
> > -	} else if (state && !ret) {
> > +		return ret;
> > +
> > +	if (state && ret)
> > +		return len;  
> 
> I would leave the original checks. It's natural pattern
> 
> 	if (foo && !bar)
> 	if (!foo && bar) // or 'else if' depending on the context
> 
> > +	if (state)
> >  		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> > -		if (ret)
> > -			goto error_ret;
> > -	}
> > +	else
> > +		ret = iio_scan_mask_clear(buffer, this_attr->address);
> > +	if (ret)
> > +		return ret;
> >  
> > -error_ret:
> > -	mutex_unlock(&iio_dev_opaque->mlock);
> > -
> > -	return ret < 0 ? ret : len;
> > +	return len;  
> 
> ...
> 
> >  	/* Already in desired state */
> >  	if (inlist == requested_state)
> > -		goto done;
> > +		return len;  
> 
> Returning error code immediately is fine, but splitting return success seems to
> me a bit too much. It is harder to follow (you really need to understand how many
> "success" variants can be).

The pattern of detecting 'nothing to do' and existing early is pretty common.
I agree that it gets dubious if there are lots of early 'success' exits, but
this one case seems reasonable to me.

Jonathan


> 
> >  	if (requested_state)
> >  		ret = __iio_update_buffers(indio_dev, buffer, NULL);
> >  	else
> >  		ret = __iio_update_buffers(indio_dev, NULL, buffer);
> > +	if (ret)
> > +		return ret;
> >  
> > -done:
> > -	mutex_unlock(&iio_dev_opaque->mlock);
> > -	return (ret < 0) ? ret : len;
> > +	return len;
> >  }  
>