Re: [PATCH v2] [media] Use common error handling code in 20 functions

[Laurent Pinchart]

Hello Markus,

On Monday, 19 February 2018 20:11:56 EET SF Markus Elfring wrote:
> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> Date: Mon, 19 Feb 2018 18:50:40 +0100
> 
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of these functions.
> 
> This issue was partly detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 
> v2:
> Hans Verkuil insisted on patch squashing. Thus several changes
> were recombined based on source files from Linux next-20180216.
> 
> The implementation of the function "tda8261_set_params" was improved
> after a notification by Christoph Böhmwalder on 2017-09-26.
> 
>  drivers/media/dvb-core/dmxdev.c                    | 16 ++++----
>  drivers/media/dvb-frontends/tda1004x.c             | 20 ++++++----
>  drivers/media/dvb-frontends/tda8261.c              | 19 ++++++----
>  drivers/media/pci/bt8xx/dst.c                      | 19 ++++++----
>  drivers/media/pci/bt8xx/dst_ca.c                   | 30 +++++++--------
>  drivers/media/pci/cx88/cx88-input.c                | 17 +++++----
>  drivers/media/platform/omap3isp/ispvideo.c         | 29 +++++++--------
>  .../media/platform/qcom/camss-8x16/camss-csid.c    | 20 +++++-----
>  drivers/media/tuners/tuner-xc2028.c                | 30 +++++++--------
>  drivers/media/usb/cpia2/cpia2_usb.c                | 13 ++++---
>  drivers/media/usb/gspca/gspca.c                    | 17 +++++----
>  drivers/media/usb/gspca/sn9c20x.c                  | 17 +++++----
>  drivers/media/usb/pvrusb2/pvrusb2-ioread.c         | 10 +++--
>  drivers/media/usb/tm6000/tm6000-cards.c            |  7 ++--
>  drivers/media/usb/tm6000/tm6000-dvb.c              | 11 ++++--
>  drivers/media/usb/tm6000/tm6000-video.c            | 13 ++++---
>  drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c  | 13 +++----
>  drivers/media/usb/ttusb-dec/ttusb_dec.c            | 43 +++++++------------
>  drivers/media/usb/uvc/uvc_v4l2.c                   | 13 ++++---
>  19 files changed, 180 insertions(+), 177 deletions(-)

[snip]

> diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> b/drivers/media/platform/omap3isp/ispvideo.c index
> a751c89a3ea8..2ddcac736402 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1315,14 +1315,12 @@ static int isp_video_open(struct file *file)
>  	/* If this is the first user, initialise the pipeline. */
>  	if (omap3isp_get(video->isp) == NULL) {
>  		ret = -EBUSY;
> -		goto done;
> +		goto delete_fh;
>  	}
> 
>  	ret = v4l2_pipeline_pm_use(&video->video.entity, 1);
> -	if (ret < 0) {
> -		omap3isp_put(video->isp);
> -		goto done;
> -	}
> +	if (ret < 0)
> +		goto put_isp;
> 
>  	queue = &handle->queue;
>  	queue->type = video->type;
> @@ -1335,10 +1333,8 @@ static int isp_video_open(struct file *file)
>  	queue->dev = video->isp->dev;
> 
>  	ret = vb2_queue_init(&handle->queue);
> -	if (ret < 0) {
> -		omap3isp_put(video->isp);
> -		goto done;
> -	}
> +	if (ret < 0)
> +		goto put_isp;
> 
>  	memset(&handle->format, 0, sizeof(handle->format));
>  	handle->format.type = video->type;
> @@ -1346,14 +1342,15 @@ static int isp_video_open(struct file *file)
> 
>  	handle->video = video;
>  	file->private_data = &handle->vfh;
> +	goto exit;

No need for a goto here, you can just return 0.

> 
> -done:
> -	if (ret < 0) {
> -		v4l2_fh_del(&handle->vfh);
> -		v4l2_fh_exit(&handle->vfh);
> -		kfree(handle);
> -	}
> -
> +put_isp:
> +	omap3isp_put(video->isp);
> +delete_fh:
> +	v4l2_fh_del(&handle->vfh);
> +	v4l2_fh_exit(&handle->vfh);
> +	kfree(handle);

Please prefix the error labels with error_.

> +exit:
>  	return ret;
>  }
> 

Otherwise the changes to omap3isp look OK to me.

[snip]

> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index a13ad4e178be..f64c851adea2 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -994,10 +994,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
> void *fh, struct v4l2_queryctrl qc = { .id = ctrl->id };
> 
>  			ret = uvc_query_v4l2_ctrl(chain, &qc);
> -			if (ret < 0) {
> -				ctrls->error_idx = i;
> -				return ret;
> -			}
> +			if (ret < 0)
> +				goto set_index;
> 
>  			ctrl->value = qc.default_value;
>  		}
> @@ -1013,14 +1011,17 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
> void *fh, ret = uvc_ctrl_get(chain, ctrl);
>  		if (ret < 0) {
>  			uvc_ctrl_rollback(handle);
> -			ctrls->error_idx = i;
> -			return ret;
> +			goto set_index;
>  		}
>  	}
> 
>  	ctrls->error_idx = 0;
> 
>  	return uvc_ctrl_rollback(handle);
> +
> +set_index:
> +	ctrls->error_idx = i;
> +	return ret;
>  }

For uvcvideo I find this to hinder readability without adding much added 
value. Please drop the uvcvideo change from this patch.

> 
>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,

-- 
Regards,

Laurent Pinchart