<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote:<br>
> V4L2Device owns a file descriptor for a v4l2 device node. It<br>
> should be managed by ScopedFD avoid the leakage.<br>
> <br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> include/libcamera/internal/v4l2_device.h | 9 +++++----<br>
> src/libcamera/v4l2_device.cpp | 17 +++++++----------<br>
> src/libcamera/v4l2_videodevice.cpp | 3 ++-<br>
> 3 files changed, 14 insertions(+), 15 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h<br>
> index d006bf68..e0262de0 100644<br>
> --- a/include/libcamera/internal/v4l2_device.h<br>
> +++ b/include/libcamera/internal/v4l2_device.h<br>
> @@ -13,6 +13,7 @@<br>
> <br>
> #include <linux/videodev2.h><br>
> <br>
> +#include <libcamera/scoped_file_descriptor.h><br>
> #include <libcamera/signal.h><br>
> <br>
> #include "libcamera/internal/log.h"<br>
> @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable<br>
> {<br>
> public:<br>
> void close();<br>
> - bool isOpen() const { return fd_ != -1; }<br>
> + bool isOpen() const { return fd_.isValid(); }<br>
> <br>
> const ControlInfoMap &controls() const { return controls_; }<br>
> <br>
> @@ -46,11 +47,11 @@ protected:<br>
> ~V4L2Device();<br>
> <br>
> int open(unsigned int flags);<br>
> - int setFd(int fd);<br>
> + int setFd(ScopedFD fd);<br>
<br>
Should this take an rvalue reference to enable move semantics ?<br>
<br></blockquote><div><br></div><div>Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to pass with move semantics.</div><div>I would take a value as if ScopedFD is unique_ptr.</div><div><br></div><div>-Hiro </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> int ioctl(unsigned long request, void *argp);<br>
> <br>
> - int fd() const { return fd_; }<br>
> + int fd() const { return fd_.get(); }<br>
> <br>
> private:<br>
> void listControls();<br>
> @@ -64,7 +65,7 @@ private:<br>
> std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;<br>
> ControlInfoMap controls_;<br>
> std::string deviceNode_;<br>
> - int fd_;<br>
> + ScopedFD fd_;<br>
> <br>
> EventNotifier *fdEventNotifier_;<br>
> bool frameStartEnabled_;<br>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp<br>
> index decd19ef..4fbb2d60 100644<br>
> --- a/src/libcamera/v4l2_device.cpp<br>
> +++ b/src/libcamera/v4l2_device.cpp<br>
> @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2)<br>
> * at open() time, and the \a logTag to prefix log messages with.<br>
> */<br>
> V4L2Device::V4L2Device(const std::string &deviceNode)<br>
> - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),<br>
> + : deviceNode_(deviceNode), fdEventNotifier_(nullptr),<br>
> frameStartEnabled_(false)<br>
> {<br>
> }<br>
> @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags)<br>
> return ret;<br>
> }<br>
> <br>
> - setFd(ret);<br>
> + setFd(ScopedFD(ret));<br>
> <br>
> listControls();<br>
> <br>
> @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags)<br>
> *<br>
> * \return 0 on success or a negative error code otherwise<br>
> */<br>
> -int V4L2Device::setFd(int fd)<br>
> +int V4L2Device::setFd(ScopedFD fd)<br>
> {<br>
> if (isOpen())<br>
> return -EBUSY;<br>
> <br>
> - fd_ = fd;<br>
> + fd_ = std::move(fd);<br>
> <br>
> - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);<br>
> + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception);<br>
> fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);<br>
> fdEventNotifier_->setEnabled(false);<br>
> <br>
> @@ -138,10 +138,7 @@ void V4L2Device::close()<br>
> <br>
> delete fdEventNotifier_;<br>
> <br>
> - if (::close(fd_) < 0)<br>
> - LOG(V4L2, Error) << "Failed to close V4L2 device: "<br>
> - << strerror(errno);<br>
> - fd_ = -1;<br>
> + fd_.reset();<br>
> }<br>
> <br>
> /**<br>
> @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp)<br>
> * Printing out an error message is usually better performed<br>
> * in the caller, which can provide more context.<br>
> */<br>
> - if (::ioctl(fd_, request, argp) < 0)<br>
> + if (::ioctl(fd_.get(), request, argp) < 0)<br>
> return -errno;<br>
> <br>
> return 0;<br>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp<br>
> index 12c09dc7..0bf3b5f5 100644<br>
> --- a/src/libcamera/v4l2_videodevice.cpp<br>
> +++ b/src/libcamera/v4l2_videodevice.cpp<br>
> @@ -22,6 +22,7 @@<br>
> #include <linux/version.h><br>
> <br>
> #include <libcamera/file_descriptor.h><br>
> +#include <libcamera/scoped_file_descriptor.h><br>
> <br>
> #include "libcamera/internal/event_notifier.h"<br>
> #include "libcamera/internal/log.h"<br>
> @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)<br>
> return ret;<br>
> }<br>
> <br>
> - ret = V4L2Device::setFd(newFd);<br>
> + ret = V4L2Device::setFd(ScopedFD(newFd));<br>
> if (ret < 0) {<br>
> LOG(V4L2, Error) << "Failed to set file handle: "<br>
> << strerror(-ret);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>