[{"id":1937,"web_url":"https://patchwork.libcamera.org/comment/1937/","msgid":"<20190619123018.GH5045@pendragon.ideasonboard.com>","date":"2019-06-19T12:30:18","subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Jun 19, 2019 at 01:05:48PM +0200, Jacopo Mondi wrote:\n> Standardize all return values checks on the 'if (ret)' style where\n> appropriate.\n\nJust curious, why did you choose (ret) over (ret < 0) ? I would have\ngone the other way around :-)\n\n> \n> No functional changes intended.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/v4l2_subdevice.cpp | 2 +-\n> src/libcamera/v4l2_videodevice.cpp | 18 +++++++++---------\n> 2 files changed, 10 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index d0e1d717b26c..7f887e377c97 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -340,7 +340,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> \tsel.r.height = rect->h;\n> \n> \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> \t\t\t<< pad << \": \" << strerror(-ret);\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index bcbac79e82e9..d1409fcafa04 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -305,11 +305,11 @@ int V4L2VideoDevice::open()\n> \tint ret;\n> \n> \tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> -\tif (ret < 0)\n> +\tif (ret)\n> \t\treturn ret;\n> \n> \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Failed to query device capabilities: \"\n> \t\t\t<< strerror(-ret);\n> @@ -633,7 +633,7 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)\n> \trb.memory = memoryType_;\n> \n> \tret = ioctl(VIDIOC_REQBUFS, &rb);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to request \" << count << \" buffers: \"\n> \t\t\t<< strerror(-ret);\n> @@ -684,7 +684,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n> \t\tbuf.m.planes = planes;\n> \n> \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> -\t\tif (ret < 0) {\n> +\t\tif (ret) {\n> \t\t\tLOG(V4L2, Error)\n> \t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> \t\t\t\t<< strerror(-ret);\n> @@ -736,7 +736,7 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,\n> \texpbuf.flags = O_RDWR;\n> \n> \tret = ioctl(VIDIOC_EXPBUF, &expbuf);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Failed to export buffer: \" << strerror(-ret);\n> \t\treturn ret;\n> @@ -927,7 +927,7 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n> \n> \tret = ioctl(VIDIOC_QBUF, &buf);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Failed to queue buffer \" << buf.index << \": \"\n> \t\t\t<< strerror(-ret);\n> @@ -963,7 +963,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> \t}\n> \n> \tret = ioctl(VIDIOC_DQBUF, &buf);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Failed to dequeue buffer: \" << strerror(-ret);\n> \t\treturn nullptr;\n> @@ -1022,7 +1022,7 @@ int V4L2VideoDevice::streamOn()\n> \tint ret;\n> \n> \tret = ioctl(VIDIOC_STREAMON, &bufferType_);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Failed to start streaming: \" << strerror(-ret);\n> \t\treturn ret;\n> @@ -1044,7 +1044,7 @@ int V4L2VideoDevice::streamOff()\n> \tint ret;\n> \n> \tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> -\tif (ret < 0) {\n> +\tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Failed to stop streaming: \" << strerror(-ret);\n> \t\treturn ret;","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59F8F61A15\n\tfor ;\n\tWed, 19 Jun 2019 14:30:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EE48E333;\n\tWed, 19 Jun 2019 14:30:34 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560947435;\n\tbh=H3AFa+geaTwK4ULj3j8Pc7dB1E26NnXq4d+lcwxfQlQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y5PUCmvz884XGq+M4iODO/sJ/y5GXZ6XjVlvE3K+a+VpAJZh7MfwkyQITxUTS9tK6\n\t4F5OmblaEAI1JcLGpwtLQaRWHCIE5ugVJmtANdNkpa58lMoIXajC4y34nI5VCIBwe0\n\tdubJUvigu8gjM4XtyK2ZBv2ER64UThbSEY4qMwfs=","Date":"Wed, 19 Jun 2019 15:30:18 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190619123018.GH5045@pendragon.ideasonboard.com>","References":"<20190619110548.20742-1-jacopo@jmondi.org>\n\t<20190619110548.20742-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190619110548.20742-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 19 Jun 2019 12:30:35 -0000"}},{"id":1939,"web_url":"https://patchwork.libcamera.org/comment/1939/","msgid":"<20190619123956.vb64he7g7zquihju@uno.localdomain>","date":"2019-06-19T12:39:56","subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Jun 19, 2019 at 03:30:18PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Jun 19, 2019 at 01:05:48PM +0200, Jacopo Mondi wrote:\n> > Standardize all return values checks on the 'if (ret)' style where\n> > appropriate.\n>\n> Just curious, why did you choose (ret) over (ret < 0) ? I would have\n> gone the other way around :-)\n>\n\nI would say tastes ? And it makes clear that the only valid result is\n0, but that does not depend on the caller.\n\nShould I drop the patch?\n\n> >\n> > No functional changes intended.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/v4l2_subdevice.cpp | 2 +-\n> > src/libcamera/v4l2_videodevice.cpp | 18 +++++++++---------\n> > 2 files changed, 10 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index d0e1d717b26c..7f887e377c97 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -340,7 +340,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > \tsel.r.height = rect->h;\n> >\n> > \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> > \t\t\t<< pad << \": \" << strerror(-ret);\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index bcbac79e82e9..d1409fcafa04 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -305,11 +305,11 @@ int V4L2VideoDevice::open()\n> > \tint ret;\n> >\n> > \tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> > -\tif (ret < 0)\n> > +\tif (ret)\n> > \t\treturn ret;\n> >\n> > \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Failed to query device capabilities: \"\n> > \t\t\t<< strerror(-ret);\n> > @@ -633,7 +633,7 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)\n> > \trb.memory = memoryType_;\n> >\n> > \tret = ioctl(VIDIOC_REQBUFS, &rb);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Unable to request \" << count << \" buffers: \"\n> > \t\t\t<< strerror(-ret);\n> > @@ -684,7 +684,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n> > \t\tbuf.m.planes = planes;\n> >\n> > \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> > -\t\tif (ret < 0) {\n> > +\t\tif (ret) {\n> > \t\t\tLOG(V4L2, Error)\n> > \t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> > \t\t\t\t<< strerror(-ret);\n> > @@ -736,7 +736,7 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,\n> > \texpbuf.flags = O_RDWR;\n> >\n> > \tret = ioctl(VIDIOC_EXPBUF, &expbuf);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Failed to export buffer: \" << strerror(-ret);\n> > \t\treturn ret;\n> > @@ -927,7 +927,7 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> > \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n> >\n> > \tret = ioctl(VIDIOC_QBUF, &buf);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Failed to queue buffer \" << buf.index << \": \"\n> > \t\t\t<< strerror(-ret);\n> > @@ -963,7 +963,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> > \t}\n> >\n> > \tret = ioctl(VIDIOC_DQBUF, &buf);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Failed to dequeue buffer: \" << strerror(-ret);\n> > \t\treturn nullptr;\n> > @@ -1022,7 +1022,7 @@ int V4L2VideoDevice::streamOn()\n> > \tint ret;\n> >\n> > \tret = ioctl(VIDIOC_STREAMON, &bufferType_);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Failed to start streaming: \" << strerror(-ret);\n> > \t\treturn ret;\n> > @@ -1044,7 +1044,7 @@ int V4L2VideoDevice::streamOff()\n> > \tint ret;\n> >\n> > \tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > -\tif (ret < 0) {\n> > +\tif (ret) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Failed to stop streaming: \" << strerror(-ret);\n> > \t\treturn ret;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 451D261A15\n\tfor ;\n\tWed, 19 Jun 2019 14:38:42 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id DF0F6200004;\n\tWed, 19 Jun 2019 12:38:41 +0000 (UTC)"],"Date":"Wed, 19 Jun 2019 14:39:56 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190619123956.vb64he7g7zquihju@uno.localdomain>","References":"<20190619110548.20742-1-jacopo@jmondi.org>\n\t<20190619110548.20742-5-jacopo@jmondi.org>\n\t<20190619123018.GH5045@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"xmrvekqxg6i7pntd\"","Content-Disposition":"inline","In-Reply-To":"<20190619123018.GH5045@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 19 Jun 2019 12:38:42 -0000"}},{"id":1940,"web_url":"https://patchwork.libcamera.org/comment/1940/","msgid":"<20190619124454.GC21753@pendragon.ideasonboard.com>","date":"2019-06-19T12:44:54","subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jun 19, 2019 at 02:39:56PM +0200, Jacopo Mondi wrote:\n> On Wed, Jun 19, 2019 at 03:30:18PM +0300, Laurent Pinchart wrote:\n> > On Wed, Jun 19, 2019 at 01:05:48PM +0200, Jacopo Mondi wrote:\n> > > Standardize all return values checks on the 'if (ret)' style where\n> > > appropriate.\n> >\n> > Just curious, why did you choose (ret) over (ret < 0) ? I would have\n> > gone the other way around :-)\n> >\n> \n> I would say tastes ? And it makes clear that the only valid result is\n> 0, but that does not depend on the caller.\n> \n> Should I drop the patch?\n\nI'm not opposed to the patch, but I'm wondering if we should make this\nlibrary-wide (in further patches possibly) and document it in our coding\nstyle.\n\nNote that a few ioctls return a non-negative value other than zero on\nsuccess. According to the man page errors are always indicated by -1.\n\n> > >\n> > > No functional changes intended.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/v4l2_subdevice.cpp | 2 +-\n> > > src/libcamera/v4l2_videodevice.cpp | 18 +++++++++---------\n> > > 2 files changed, 10 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > index d0e1d717b26c..7f887e377c97 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -340,7 +340,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > \tsel.r.height = rect->h;\n> > >\n> > > \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> > > \t\t\t<< pad << \": \" << strerror(-ret);\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index bcbac79e82e9..d1409fcafa04 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -305,11 +305,11 @@ int V4L2VideoDevice::open()\n> > > \tint ret;\n> > >\n> > > \tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> > > -\tif (ret < 0)\n> > > +\tif (ret)\n> > > \t\treturn ret;\n> > >\n> > > \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Failed to query device capabilities: \"\n> > > \t\t\t<< strerror(-ret);\n> > > @@ -633,7 +633,7 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)\n> > > \trb.memory = memoryType_;\n> > >\n> > > \tret = ioctl(VIDIOC_REQBUFS, &rb);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Unable to request \" << count << \" buffers: \"\n> > > \t\t\t<< strerror(-ret);\n> > > @@ -684,7 +684,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n> > > \t\tbuf.m.planes = planes;\n> > >\n> > > \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> > > -\t\tif (ret < 0) {\n> > > +\t\tif (ret) {\n> > > \t\t\tLOG(V4L2, Error)\n> > > \t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> > > \t\t\t\t<< strerror(-ret);\n> > > @@ -736,7 +736,7 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,\n> > > \texpbuf.flags = O_RDWR;\n> > >\n> > > \tret = ioctl(VIDIOC_EXPBUF, &expbuf);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Failed to export buffer: \" << strerror(-ret);\n> > > \t\treturn ret;\n> > > @@ -927,7 +927,7 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> > > \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n> > >\n> > > \tret = ioctl(VIDIOC_QBUF, &buf);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Failed to queue buffer \" << buf.index << \": \"\n> > > \t\t\t<< strerror(-ret);\n> > > @@ -963,7 +963,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> > > \t}\n> > >\n> > > \tret = ioctl(VIDIOC_DQBUF, &buf);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Failed to dequeue buffer: \" << strerror(-ret);\n> > > \t\treturn nullptr;\n> > > @@ -1022,7 +1022,7 @@ int V4L2VideoDevice::streamOn()\n> > > \tint ret;\n> > >\n> > > \tret = ioctl(VIDIOC_STREAMON, &bufferType_);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Failed to start streaming: \" << strerror(-ret);\n> > > \t\treturn ret;\n> > > @@ -1044,7 +1044,7 @@ int V4L2VideoDevice::streamOff()\n> > > \tint ret;\n> > >\n> > > \tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > > -\tif (ret < 0) {\n> > > +\tif (ret) {\n> > > \t\tLOG(V4L2, Error)\n> > > \t\t\t<< \"Failed to stop streaming: \" << strerror(-ret);\n> > > \t\treturn ret;","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C98161A15\n\tfor ;\n\tWed, 19 Jun 2019 14:45:12 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B7429333;\n\tWed, 19 Jun 2019 14:45:11 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560948311;\n\tbh=HKstg72aWboAJ8jMxhpTfIRiaOmRWNZHw+GybQfwZec=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p25PPJxe3XfeXvst6427ETs+L9oV0O30Lo9MDSIvUzF/S6OPE36R+t5gxhnQTGQZV\n\tYNaSc086dFykICLGAZBSMSwvn7Dr5MMykPZcU3DsqiMJ7+yrlLS5oNG9T0BnBYlUO9\n\tXVJutv6MOXcYlX21GgJbQcoYAXZQOea8XNAIsaUs=","Date":"Wed, 19 Jun 2019 15:44:54 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190619124454.GC21753@pendragon.ideasonboard.com>","References":"<20190619110548.20742-1-jacopo@jmondi.org>\n\t<20190619110548.20742-5-jacopo@jmondi.org>\n\t<20190619123018.GH5045@pendragon.ideasonboard.com>\n\t<20190619123956.vb64he7g7zquihju@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190619123956.vb64he7g7zquihju@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 19 Jun 2019 12:45:12 -0000"}},{"id":1941,"web_url":"https://patchwork.libcamera.org/comment/1941/","msgid":"<20190619130303.qdlo2q5q5gtov2e2@uno.localdomain>","date":"2019-06-19T13:03:03","subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Jun 19, 2019 at 03:44:54PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Jun 19, 2019 at 02:39:56PM +0200, Jacopo Mondi wrote:\n> > On Wed, Jun 19, 2019 at 03:30:18PM +0300, Laurent Pinchart wrote:\n> > > On Wed, Jun 19, 2019 at 01:05:48PM +0200, Jacopo Mondi wrote:\n> > > > Standardize all return values checks on the 'if (ret)' style where\n> > > > appropriate.\n> > >\n> > > Just curious, why did you choose (ret) over (ret < 0) ? I would have\n> > > gone the other way around :-)\n> > >\n> >\n> > I would say tastes ? And it makes clear that the only valid result is\n> > 0, but that does not depend on the caller.\n> >\n> > Should I drop the patch?\n>\n> I'm not opposed to the patch, but I'm wondering if we should make this\n> library-wide (in further patches possibly) and document it in our coding\n> style.\n>\n> Note that a few ioctls return a non-negative value other than zero on\n> success. According to the man page errors are always indicated by -1.\n>\n\nFor the ioctl system call that's indeed true, but here we're calling\nthe base class V4L2Device::ioctl() operation, which returns 0 on\nsuccess.\n\nAnyway, no need to spend too much time on this. I'll drop the patch.\n\nThanks\n j\n> > > >\n> > > > No functional changes intended.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/v4l2_subdevice.cpp | 2 +-\n> > > > src/libcamera/v4l2_videodevice.cpp | 18 +++++++++---------\n> > > > 2 files changed, 10 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > > > index d0e1d717b26c..7f887e377c97 100644\n> > > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > > @@ -340,7 +340,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > > > \tsel.r.height = rect->h;\n> > > >\n> > > > \tint ret = ioctl(VIDIOC_SUBDEV_S_SELECTION, &sel);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Unable to set rectangle \" << target << \" on pad \"\n> > > > \t\t\t<< pad << \": \" << strerror(-ret);\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index bcbac79e82e9..d1409fcafa04 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -305,11 +305,11 @@ int V4L2VideoDevice::open()\n> > > > \tint ret;\n> > > >\n> > > > \tret = V4L2Device::open(O_RDWR | O_NONBLOCK);\n> > > > -\tif (ret < 0)\n> > > > +\tif (ret)\n> > > > \t\treturn ret;\n> > > >\n> > > > \tret = ioctl(VIDIOC_QUERYCAP, &caps_);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Failed to query device capabilities: \"\n> > > > \t\t\t<< strerror(-ret);\n> > > > @@ -633,7 +633,7 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)\n> > > > \trb.memory = memoryType_;\n> > > >\n> > > > \tret = ioctl(VIDIOC_REQBUFS, &rb);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Unable to request \" << count << \" buffers: \"\n> > > > \t\t\t<< strerror(-ret);\n> > > > @@ -684,7 +684,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n> > > > \t\tbuf.m.planes = planes;\n> > > >\n> > > > \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> > > > -\t\tif (ret < 0) {\n> > > > +\t\tif (ret) {\n> > > > \t\t\tLOG(V4L2, Error)\n> > > > \t\t\t\t<< \"Unable to query buffer \" << i << \": \"\n> > > > \t\t\t\t<< strerror(-ret);\n> > > > @@ -736,7 +736,7 @@ int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,\n> > > > \texpbuf.flags = O_RDWR;\n> > > >\n> > > > \tret = ioctl(VIDIOC_EXPBUF, &expbuf);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Failed to export buffer: \" << strerror(-ret);\n> > > > \t\treturn ret;\n> > > > @@ -927,7 +927,7 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> > > > \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n> > > >\n> > > > \tret = ioctl(VIDIOC_QBUF, &buf);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Failed to queue buffer \" << buf.index << \": \"\n> > > > \t\t\t<< strerror(-ret);\n> > > > @@ -963,7 +963,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> > > > \t}\n> > > >\n> > > > \tret = ioctl(VIDIOC_DQBUF, &buf);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Failed to dequeue buffer: \" << strerror(-ret);\n> > > > \t\treturn nullptr;\n> > > > @@ -1022,7 +1022,7 @@ int V4L2VideoDevice::streamOn()\n> > > > \tint ret;\n> > > >\n> > > > \tret = ioctl(VIDIOC_STREAMON, &bufferType_);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Failed to start streaming: \" << strerror(-ret);\n> > > > \t\treturn ret;\n> > > > @@ -1044,7 +1044,7 @@ int V4L2VideoDevice::streamOff()\n> > > > \tint ret;\n> > > >\n> > > > \tret = ioctl(VIDIOC_STREAMOFF, &bufferType_);\n> > > > -\tif (ret < 0) {\n> > > > +\tif (ret) {\n> > > > \t\tLOG(V4L2, Error)\n> > > > \t\t\t<< \"Failed to stop streaming: \" << strerror(-ret);\n> > > > \t\treturn ret;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F3A361A15\n\tfor ;\n\tWed, 19 Jun 2019 15:01:50 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 77CE824000F;\n\tWed, 19 Jun 2019 13:01:49 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 19 Jun 2019 15:03:03 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190619130303.qdlo2q5q5gtov2e2@uno.localdomain>","References":"<20190619110548.20742-1-jacopo@jmondi.org>\n\t<20190619110548.20742-5-jacopo@jmondi.org>\n\t<20190619123018.GH5045@pendragon.ideasonboard.com>\n\t<20190619123956.vb64he7g7zquihju@uno.localdomain>\n\t<20190619124454.GC21753@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"rpk6arhqrohymcw3\"","Content-Disposition":"inline","In-Reply-To":"<20190619124454.GC21753@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 4/4] libcamera: v4l2: Standardize\n\treturn value checks","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 19 Jun 2019 13:01:50 -0000"}}]