From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from lists.s-osg.org ([54.187.51.154]:55704 "EHLO lists.s-osg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752076AbcDMT5U (ORCPT ); Wed, 13 Apr 2016 15:57:20 -0400 Date: Wed, 13 Apr 2016 16:57:14 -0300 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: linux-media@vger.kernel.org, Niklas =?UTF-8?B?U8O2ZGVybHVuZA==?= , Hans Verkuil Subject: Re: [PATCHv2 1/2] v4l2-ioctl: simplify code Message-ID: <20160413165714.587c5e7c@recife.lan> In-Reply-To: <1458550080-42743-2-git-send-email-hverkuil@xs4all.nl> References: <1458550080-42743-1-git-send-email-hverkuil@xs4all.nl> <1458550080-42743-2-git-send-email-hverkuil@xs4all.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Mon, 21 Mar 2016 09:47:59 +0100 Hans Verkuil escreveu: > From: Hans Verkuil > > Instead of a big if at the beginning, just check if g_selection == NULL > and call the cropcap op immediately and return the result. > > No functional changes in this patch. Hmm... not true. See below. > > Signed-off-by: Hans Verkuil You forgot to add Miklas review here. > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++---------------- > 1 file changed, 29 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 6bf5a3e..3cf8d3a 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -2160,33 +2160,40 @@ static int v4l_cropcap(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > struct v4l2_cropcap *p = arg; > + struct v4l2_selection s = { .type = p->type }; > + int ret; > > - if (ops->vidioc_g_selection) { > - struct v4l2_selection s = { .type = p->type }; > - int ret; > + if (ops->vidioc_g_selection == NULL) { > + /* > + * The determine_valid_ioctls() call already should ensure > + * that ops->vidioc_cropcap != NULL, but just in case... > + */ > + if (ops->vidioc_cropcap) > + return ops->vidioc_cropcap(file, fh, p); > + return -ENOTTY; Actually, before this patch, the logic would be doing, instead: /* setting trivial pixelaspect */ p->pixelaspect.numerator = 1; p->pixelaspect.denominator = 1; if (ops->vidioc_cropcap) return ops->vidioc_cropcap(file, fh, p); return 0; With is not the same as what's there. So, there are actually two differences: 1) it will now return -ENOTTY if !ops->vidioc_cropcap instead of 0. (returning -ENOTTY is probably the best, but this is a fixup change, and a functional change, so, should be in a separate patch, if needed) 2) pixel numerator/denominator is not initialized in the new code. Ok, maybe all drivers would be setting numerator and denominator to 1 as default, so the code won't need to do it, but: a) This is not a trivial assumption by looking only on this patch; b) It sounds risky to not setup denominator, as, if a driver is not initializing it, it could cause a division by zero at userspace. So, I would prefer to keep initializing numerator/denominator here. As this is just a cleanup patch, I'm skipping it for now. Btw, IMHO, the best here would be to split the logic that it is called if ops->vidioc_g_selection is not NULL on a separate function. That would make the logic even clearer, and would help to show that it will be handling the logic below "setting trivial pixelaspect" the same way. Regards, Mauro > + } > > - /* obtaining bounds */ > - if (V4L2_TYPE_IS_OUTPUT(p->type)) > - s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS; > - else > - s.target = V4L2_SEL_TGT_CROP_BOUNDS; > + /* obtaining bounds */ > + if (V4L2_TYPE_IS_OUTPUT(p->type)) > + s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS; > + else > + s.target = V4L2_SEL_TGT_CROP_BOUNDS; > > - ret = ops->vidioc_g_selection(file, fh, &s); > - if (ret) > - return ret; > - p->bounds = s.r; > + ret = ops->vidioc_g_selection(file, fh, &s); > + if (ret) > + return ret; > + p->bounds = s.r; > > - /* obtaining defrect */ > - if (V4L2_TYPE_IS_OUTPUT(p->type)) > - s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT; > - else > - s.target = V4L2_SEL_TGT_CROP_DEFAULT; > + /* obtaining defrect */ > + if (V4L2_TYPE_IS_OUTPUT(p->type)) > + s.target = V4L2_SEL_TGT_COMPOSE_DEFAULT; > + else > + s.target = V4L2_SEL_TGT_CROP_DEFAULT; > > - ret = ops->vidioc_g_selection(file, fh, &s); > - if (ret) > - return ret; > - p->defrect = s.r; > - } > + ret = ops->vidioc_g_selection(file, fh, &s); > + if (ret) > + return ret; > + p->defrect = s.r; > > /* setting trivial pixelaspect */ > p->pixelaspect.numerator = 1; -- Thanks, Mauro