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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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).

>  	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;
>  }

-- 
With Best Regards,
Andy Shevchenko