| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 19 Feb 2018 20:37:29 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc: linux-media@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
Andi Shyti <andi.shyti@...sung.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Andrey Utkin <andrey_utkin@...tmail.com>,
Arvind Yadav <arvind.yadav.cs@...il.com>,
Bhumika Goyal <bhumirks@...il.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Brian Johnson <brijohn@...il.com>,
Christoph Böhmwalder
<christoph@...hmwalder.at>,
Christophe Jaillet <christophe.jaillet@...adoo.fr>,
Colin Ian King <colin.king@...onical.com>,
Daniele Nicolodi <daniele@...nta.net>,
David Härdeman <david@...deman.nu>,
Devendra Sharma <devendra.sharma9091@...il.com>,
"Gustavo A. R. Silva" <garsilva@...eddedor.com>,
Hans Verkuil <hans.verkuil@...co.com>,
Inki Dae <inki.dae@...sung.com>, Joe Perches <joe@...ches.com>,
Kees Cook <keescook@...omium.org>,
Masahiro Yamada <yamada.masahiro@...ionext.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Max Kellermann <max.kellermann@...il.com>,
Mike Isely <isely@...ox.com>,
Philippe Ombredanne <pombredanne@...b.com>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Santosh Kumar Singh <kumar.san1093@...il.com>,
Satendra Singh Thakur <satendra.t@...sung.com>,
Sean Young <sean@...s.org>,
Seung-Woo Kim <sw0312.kim@...sung.com>,
Shyam Saini <mayhs11saini@...il.com>,
Thomas Gleixner <tglx@...utronix.de>,
Todor Tomov <todor.tomov@...aro.org>,
Wei Yongjun <weiyongjun1@...wei.com>,
LKML <linux-kernel@...r.kernel.org>,
kernel-janitors@...r.kernel.org
Subject: Re: [PATCH v2] [media] Use common error handling code in 20 functions
Hello Markus,
On Monday, 19 February 2018 20:11:56 EET SF Markus Elfring wrote:
> From: Markus Elfring <elfring@...rs.sourceforge.net>
> 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@...rs.sourceforge.net>
> ---
>
> 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
Powered by blists - more mailing lists