| Message ID | 20210415083843.3399502-6-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote: > V4L2Device owns a file descriptor for a v4l2 device node. It > should be managed by ScopedFD avoid the leakage. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/v4l2_device.h | 9 +++++---- > src/libcamera/v4l2_device.cpp | 17 +++++++---------- > src/libcamera/v4l2_videodevice.cpp | 3 ++- > 3 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index d006bf68..e0262de0 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -13,6 +13,7 @@ > > #include <linux/videodev2.h> > > +#include <libcamera/scoped_file_descriptor.h> > #include <libcamera/signal.h> > > #include "libcamera/internal/log.h" > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable > { > public: > void close(); > - bool isOpen() const { return fd_ != -1; } > + bool isOpen() const { return fd_.isValid(); } > > const ControlInfoMap &controls() const { return controls_; } > > @@ -46,11 +47,11 @@ protected: > ~V4L2Device(); > > int open(unsigned int flags); > - int setFd(int fd); > + int setFd(ScopedFD fd); Should this take an rvalue reference to enable move semantics ? > > int ioctl(unsigned long request, void *argp); > > - int fd() const { return fd_; } > + int fd() const { return fd_.get(); } > > private: > void listControls(); > @@ -64,7 +65,7 @@ private: > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > ControlInfoMap controls_; > std::string deviceNode_; > - int fd_; > + ScopedFD fd_; > > EventNotifier *fdEventNotifier_; > bool frameStartEnabled_; > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index decd19ef..4fbb2d60 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2) > * at open() time, and the \a logTag to prefix log messages with. > */ > V4L2Device::V4L2Device(const std::string &deviceNode) > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr), > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr), > frameStartEnabled_(false) > { > } > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags) > return ret; > } > > - setFd(ret); > + setFd(ScopedFD(ret)); > > listControls(); > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags) > * > * \return 0 on success or a negative error code otherwise > */ > -int V4L2Device::setFd(int fd) > +int V4L2Device::setFd(ScopedFD fd) > { > if (isOpen()) > return -EBUSY; > > - fd_ = fd; > + fd_ = std::move(fd); > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception); > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception); > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable); > fdEventNotifier_->setEnabled(false); > > @@ -138,10 +138,7 @@ void V4L2Device::close() > > delete fdEventNotifier_; > > - if (::close(fd_) < 0) > - LOG(V4L2, Error) << "Failed to close V4L2 device: " > - << strerror(errno); > - fd_ = -1; > + fd_.reset(); > } > > /** > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp) > * Printing out an error message is usually better performed > * in the caller, which can provide more context. > */ > - if (::ioctl(fd_, request, argp) < 0) > + if (::ioctl(fd_.get(), request, argp) < 0) > return -errno; > > return 0; > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 12c09dc7..0bf3b5f5 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -22,6 +22,7 @@ > #include <linux/version.h> > > #include <libcamera/file_descriptor.h> > +#include <libcamera/scoped_file_descriptor.h> > > #include "libcamera/internal/event_notifier.h" > #include "libcamera/internal/log.h" > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > return ret; > } > > - ret = V4L2Device::setFd(newFd); > + ret = V4L2Device::setFd(ScopedFD(newFd)); > if (ret < 0) { > LOG(V4L2, Error) << "Failed to set file handle: " > << strerror(-ret);
Hi Laurent, On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Hiro, > > Thank you for the patch. > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote: > > V4L2Device owns a file descriptor for a v4l2 device node. It > > should be managed by ScopedFD avoid the leakage. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/v4l2_device.h | 9 +++++---- > > src/libcamera/v4l2_device.cpp | 17 +++++++---------- > > src/libcamera/v4l2_videodevice.cpp | 3 ++- > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_device.h > b/include/libcamera/internal/v4l2_device.h > > index d006bf68..e0262de0 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -13,6 +13,7 @@ > > > > #include <linux/videodev2.h> > > > > +#include <libcamera/scoped_file_descriptor.h> > > #include <libcamera/signal.h> > > > > #include "libcamera/internal/log.h" > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable > > { > > public: > > void close(); > > - bool isOpen() const { return fd_ != -1; } > > + bool isOpen() const { return fd_.isValid(); } > > > > const ControlInfoMap &controls() const { return controls_; } > > > > @@ -46,11 +47,11 @@ protected: > > ~V4L2Device(); > > > > int open(unsigned int flags); > > - int setFd(int fd); > > + int setFd(ScopedFD fd); > > Should this take an rvalue reference to enable move semantics ? > > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to pass with move semantics. I would take a value as if ScopedFD is unique_ptr. -Hiro > > > > int ioctl(unsigned long request, void *argp); > > > > - int fd() const { return fd_; } > > + int fd() const { return fd_.get(); } > > > > private: > > void listControls(); > > @@ -64,7 +65,7 @@ private: > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > ControlInfoMap controls_; > > std::string deviceNode_; > > - int fd_; > > + ScopedFD fd_; > > > > EventNotifier *fdEventNotifier_; > > bool frameStartEnabled_; > > diff --git a/src/libcamera/v4l2_device.cpp > b/src/libcamera/v4l2_device.cpp > > index decd19ef..4fbb2d60 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2) > > * at open() time, and the \a logTag to prefix log messages with. > > */ > > V4L2Device::V4L2Device(const std::string &deviceNode) > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr), > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr), > > frameStartEnabled_(false) > > { > > } > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags) > > return ret; > > } > > > > - setFd(ret); > > + setFd(ScopedFD(ret)); > > > > listControls(); > > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags) > > * > > * \return 0 on success or a negative error code otherwise > > */ > > -int V4L2Device::setFd(int fd) > > +int V4L2Device::setFd(ScopedFD fd) > > { > > if (isOpen()) > > return -EBUSY; > > > > - fd_ = fd; > > + fd_ = std::move(fd); > > > > - fdEventNotifier_ = new EventNotifier(fd_, > EventNotifier::Exception); > > + fdEventNotifier_ = new EventNotifier(fd_.get(), > EventNotifier::Exception); > > fdEventNotifier_->activated.connect(this, > &V4L2Device::eventAvailable); > > fdEventNotifier_->setEnabled(false); > > > > @@ -138,10 +138,7 @@ void V4L2Device::close() > > > > delete fdEventNotifier_; > > > > - if (::close(fd_) < 0) > > - LOG(V4L2, Error) << "Failed to close V4L2 device: " > > - << strerror(errno); > > - fd_ = -1; > > + fd_.reset(); > > } > > > > /** > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void > *argp) > > * Printing out an error message is usually better performed > > * in the caller, which can provide more context. > > */ > > - if (::ioctl(fd_, request, argp) < 0) > > + if (::ioctl(fd_.get(), request, argp) < 0) > > return -errno; > > > > return 0; > > diff --git a/src/libcamera/v4l2_videodevice.cpp > b/src/libcamera/v4l2_videodevice.cpp > > index 12c09dc7..0bf3b5f5 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -22,6 +22,7 @@ > > #include <linux/version.h> > > > > #include <libcamera/file_descriptor.h> > > +#include <libcamera/scoped_file_descriptor.h> > > > > #include "libcamera/internal/event_notifier.h" > > #include "libcamera/internal/log.h" > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum > v4l2_buf_type type) > > return ret; > > } > > > > - ret = V4L2Device::setFd(newFd); > > + ret = V4L2Device::setFd(ScopedFD(newFd)); > > if (ret < 0) { > > LOG(V4L2, Error) << "Failed to set file handle: " > > << strerror(-ret); > > -- > Regards, > > Laurent Pinchart >
Hi Hiro, On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote: > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote: > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote: > > > V4L2Device owns a file descriptor for a v4l2 device node. It > > > should be managed by ScopedFD avoid the leakage. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > include/libcamera/internal/v4l2_device.h | 9 +++++---- > > > src/libcamera/v4l2_device.cpp | 17 +++++++---------- > > > src/libcamera/v4l2_videodevice.cpp | 3 ++- > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > index d006bf68..e0262de0 100644 > > > --- a/include/libcamera/internal/v4l2_device.h > > > +++ b/include/libcamera/internal/v4l2_device.h > > > @@ -13,6 +13,7 @@ > > > > > > #include <linux/videodev2.h> > > > > > > +#include <libcamera/scoped_file_descriptor.h> > > > #include <libcamera/signal.h> > > > > > > #include "libcamera/internal/log.h" > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable > > > { > > > public: > > > void close(); > > > - bool isOpen() const { return fd_ != -1; } > > > + bool isOpen() const { return fd_.isValid(); } > > > > > > const ControlInfoMap &controls() const { return controls_; } > > > > > > @@ -46,11 +47,11 @@ protected: > > > ~V4L2Device(); > > > > > > int open(unsigned int flags); > > > - int setFd(int fd); > > > + int setFd(ScopedFD fd); > > > > Should this take an rvalue reference to enable move semantics ? > > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to > pass with move semantics. This means that a new ScopedFD instance will be created using the move constructor, to be passed to the setFd() function, which will internally use the move assignment operator. We can avoid the move constructor by passing an rvalue reference here. It would also clearly indicate to the user that a move is the expected behaviour of setFd(), while this is currently implicit only, as a consequence of ScopedFD deleting the copy constructor. > I would take a value as if ScopedFD is unique_ptr. > > > > > > > int ioctl(unsigned long request, void *argp); > > > > > > - int fd() const { return fd_; } > > > + int fd() const { return fd_.get(); } > > > > > > private: > > > void listControls(); > > > @@ -64,7 +65,7 @@ private: > > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > > ControlInfoMap controls_; > > > std::string deviceNode_; > > > - int fd_; > > > + ScopedFD fd_; > > > > > > EventNotifier *fdEventNotifier_; > > > bool frameStartEnabled_; > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index decd19ef..4fbb2d60 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2) > > > * at open() time, and the \a logTag to prefix log messages with. > > > */ > > > V4L2Device::V4L2Device(const std::string &deviceNode) > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr), > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr), > > > frameStartEnabled_(false) > > > { > > > } > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags) > > > return ret; > > > } > > > > > > - setFd(ret); > > > + setFd(ScopedFD(ret)); > > > > > > listControls(); > > > > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags) > > > * > > > * \return 0 on success or a negative error code otherwise > > > */ > > > -int V4L2Device::setFd(int fd) > > > +int V4L2Device::setFd(ScopedFD fd) > > > { > > > if (isOpen()) > > > return -EBUSY; > > > > > > - fd_ = fd; > > > + fd_ = std::move(fd); > > > > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception); > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception); > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable); > > > fdEventNotifier_->setEnabled(false); > > > > > > @@ -138,10 +138,7 @@ void V4L2Device::close() > > > > > > delete fdEventNotifier_; > > > > > > - if (::close(fd_) < 0) > > > - LOG(V4L2, Error) << "Failed to close V4L2 device: " > > > - << strerror(errno); > > > - fd_ = -1; > > > + fd_.reset(); > > > } > > > > > > /** > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp) > > > * Printing out an error message is usually better performed > > > * in the caller, which can provide more context. > > > */ > > > - if (::ioctl(fd_, request, argp) < 0) > > > + if (::ioctl(fd_.get(), request, argp) < 0) > > > return -errno; > > > > > > return 0; > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index 12c09dc7..0bf3b5f5 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -22,6 +22,7 @@ > > > #include <linux/version.h> > > > > > > #include <libcamera/file_descriptor.h> > > > +#include <libcamera/scoped_file_descriptor.h> > > > > > > #include "libcamera/internal/event_notifier.h" > > > #include "libcamera/internal/log.h" > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > > > return ret; > > > } > > > > > > - ret = V4L2Device::setFd(newFd); > > > + ret = V4L2Device::setFd(ScopedFD(newFd)); > > > if (ret < 0) { > > > LOG(V4L2, Error) << "Failed to set file handle: " > > > << strerror(-ret);
Hi Laurent, On Mon, Jun 28, 2021 at 11:50 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote: > > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote: > > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote: > > > > V4L2Device owns a file descriptor for a v4l2 device node. It > > > > should be managed by ScopedFD avoid the leakage. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > include/libcamera/internal/v4l2_device.h | 9 +++++---- > > > > src/libcamera/v4l2_device.cpp | 17 +++++++---------- > > > > src/libcamera/v4l2_videodevice.cpp | 3 ++- > > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > > index d006bf68..e0262de0 100644 > > > > --- a/include/libcamera/internal/v4l2_device.h > > > > +++ b/include/libcamera/internal/v4l2_device.h > > > > @@ -13,6 +13,7 @@ > > > > > > > > #include <linux/videodev2.h> > > > > > > > > +#include <libcamera/scoped_file_descriptor.h> > > > > #include <libcamera/signal.h> > > > > > > > > #include "libcamera/internal/log.h" > > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable > > > > { > > > > public: > > > > void close(); > > > > - bool isOpen() const { return fd_ != -1; } > > > > + bool isOpen() const { return fd_.isValid(); } > > > > > > > > const ControlInfoMap &controls() const { return controls_; } > > > > > > > > @@ -46,11 +47,11 @@ protected: > > > > ~V4L2Device(); > > > > > > > > int open(unsigned int flags); > > > > - int setFd(int fd); > > > > + int setFd(ScopedFD fd); > > > > > > Should this take an rvalue reference to enable move semantics ? > > > > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to > > pass with move semantics. > > This means that a new ScopedFD instance will be created using the move > constructor, to be passed to the setFd() function, which will internally > use the move assignment operator. We can avoid the move constructor by > passing an rvalue reference here. It would also clearly indicate to the > user that a move is the expected behaviour of setFd(), while this is > currently implicit only, as a consequence of ScopedFD deleting the copy > constructor. > You're right. If we think about the overhead, we should avoid this pass-by-value. I generally pass-by-value to clarify the ownership. There is no chance that a caller's ScopedFD is valid when the function returns. With pass-by-rvalue-reference, if a function doesn't invalidate the ScopedFD, it can still be valid when the function returns. I think that's why Google C++ style guide suggests rvalue reference in specific functions only. https://google.github.io/styleguide/cppguide.html#Rvalue_references Although setFd can be regarded as "&&-qualified methods that logically "consume" *this". I am fine to use rvalue reference here as setFd should consume the passed ScopedFD. What do you think? -Hiro > > I would take a value as if ScopedFD is unique_ptr. > > > > > > > > > > int ioctl(unsigned long request, void *argp); > > > > > > > > - int fd() const { return fd_; } > > > > + int fd() const { return fd_.get(); } > > > > > > > > private: > > > > void listControls(); > > > > @@ -64,7 +65,7 @@ private: > > > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > > > ControlInfoMap controls_; > > > > std::string deviceNode_; > > > > - int fd_; > > > > + ScopedFD fd_; > > > > > > > > EventNotifier *fdEventNotifier_; > > > > bool frameStartEnabled_; > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > index decd19ef..4fbb2d60 100644 > > > > --- a/src/libcamera/v4l2_device.cpp > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2) > > > > * at open() time, and the \a logTag to prefix log messages with. > > > > */ > > > > V4L2Device::V4L2Device(const std::string &deviceNode) > > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr), > > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr), > > > > frameStartEnabled_(false) > > > > { > > > > } > > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags) > > > > return ret; > > > > } > > > > > > > > - setFd(ret); > > > > + setFd(ScopedFD(ret)); > > > > > > > > listControls(); > > > > > > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags) > > > > * > > > > * \return 0 on success or a negative error code otherwise > > > > */ > > > > -int V4L2Device::setFd(int fd) > > > > +int V4L2Device::setFd(ScopedFD fd) > > > > { > > > > if (isOpen()) > > > > return -EBUSY; > > > > > > > > - fd_ = fd; > > > > + fd_ = std::move(fd); > > > > > > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception); > > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception); > > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable); > > > > fdEventNotifier_->setEnabled(false); > > > > > > > > @@ -138,10 +138,7 @@ void V4L2Device::close() > > > > > > > > delete fdEventNotifier_; > > > > > > > > - if (::close(fd_) < 0) > > > > - LOG(V4L2, Error) << "Failed to close V4L2 device: " > > > > - << strerror(errno); > > > > - fd_ = -1; > > > > + fd_.reset(); > > > > } > > > > > > > > /** > > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp) > > > > * Printing out an error message is usually better performed > > > > * in the caller, which can provide more context. > > > > */ > > > > - if (::ioctl(fd_, request, argp) < 0) > > > > + if (::ioctl(fd_.get(), request, argp) < 0) > > > > return -errno; > > > > > > > > return 0; > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > index 12c09dc7..0bf3b5f5 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -22,6 +22,7 @@ > > > > #include <linux/version.h> > > > > > > > > #include <libcamera/file_descriptor.h> > > > > +#include <libcamera/scoped_file_descriptor.h> > > > > > > > > #include "libcamera/internal/event_notifier.h" > > > > #include "libcamera/internal/log.h" > > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > > > > return ret; > > > > } > > > > > > > > - ret = V4L2Device::setFd(newFd); > > > > + ret = V4L2Device::setFd(ScopedFD(newFd)); > > > > if (ret < 0) { > > > > LOG(V4L2, Error) << "Failed to set file handle: " > > > > << strerror(-ret); > > -- > Regards, > > Laurent Pinchart
Replying to an old e-mail, On Mon, Jun 28, 2021 at 05:49:56AM +0300, Laurent Pinchart wrote: > On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote: > > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote: > > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote: > > > > V4L2Device owns a file descriptor for a v4l2 device node. It > > > > should be managed by ScopedFD avoid the leakage. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > include/libcamera/internal/v4l2_device.h | 9 +++++---- > > > > src/libcamera/v4l2_device.cpp | 17 +++++++---------- > > > > src/libcamera/v4l2_videodevice.cpp | 3 ++- > > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > > index d006bf68..e0262de0 100644 > > > > --- a/include/libcamera/internal/v4l2_device.h > > > > +++ b/include/libcamera/internal/v4l2_device.h > > > > @@ -13,6 +13,7 @@ > > > > > > > > #include <linux/videodev2.h> > > > > > > > > +#include <libcamera/scoped_file_descriptor.h> > > > > #include <libcamera/signal.h> > > > > > > > > #include "libcamera/internal/log.h" > > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable > > > > { > > > > public: > > > > void close(); > > > > - bool isOpen() const { return fd_ != -1; } > > > > + bool isOpen() const { return fd_.isValid(); } > > > > > > > > const ControlInfoMap &controls() const { return controls_; } > > > > > > > > @@ -46,11 +47,11 @@ protected: > > > > ~V4L2Device(); > > > > > > > > int open(unsigned int flags); > > > > - int setFd(int fd); > > > > + int setFd(ScopedFD fd); > > > > > > Should this take an rvalue reference to enable move semantics ? > > > > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to > > pass with move semantics. > > This means that a new ScopedFD instance will be created using the move > constructor, to be passed to the setFd() function, which will internally > use the move assignment operator. We can avoid the move constructor by > passing an rvalue reference here. It would also clearly indicate to the > user that a move is the expected behaviour of setFd(), while this is > currently implicit only, as a consequence of ScopedFD deleting the copy > constructor. It recently came to my attention that passing a std::unique_ptr<> by rvalue reference is usually a bad idea. The class is cheap to move, and with a pass-by-value call, it's clear that move is being implemented as std::unique_ptr<> has no copy constructor. When passing by rvalue reference, the caller has to use std::move(), but it's up to the callee to actually perform the move. Compare the following: void foo(std::unique_ptr<Foo> ptr) { } void foo(std::unique_ptr<Foo> &&ptr) { } with foo being called as std::unique_ptr<Foo> f = ...; foo(std::move(f)); In the first case, ptr is constructed by moving f. ptr gets destroyed at the end of foo(), and because foo() is empty, it still owns the pointer, which then gets deleted. When foo() returns, f doesn't own a pointer anymore. In the second case, f will still own the pointer after foo() returns. Of course, if foo() was implemented as void foo(std::unique_ptr<Foo> &&ptr) { globalPtr = std::move(ptr); } then f would not own the pointer after foo() returns. The point here is that passing a std::unique_ptr<> rvalue reference makes the behaviour more dependent on the implementation of the function, while passing a value ensures that f will not own the pointer anymore when foo() returns, regardless of how it's implemented. The same applies to ScopedFD. Of course, if the class was expensive to move, it would be a different cases. > > I would take a value as if ScopedFD is unique_ptr. > > > > > > > > > > int ioctl(unsigned long request, void *argp); > > > > > > > > - int fd() const { return fd_; } > > > > + int fd() const { return fd_.get(); } > > > > > > > > private: > > > > void listControls(); > > > > @@ -64,7 +65,7 @@ private: > > > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > > > ControlInfoMap controls_; > > > > std::string deviceNode_; > > > > - int fd_; > > > > + ScopedFD fd_; > > > > > > > > EventNotifier *fdEventNotifier_; > > > > bool frameStartEnabled_; > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > index decd19ef..4fbb2d60 100644 > > > > --- a/src/libcamera/v4l2_device.cpp > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2) > > > > * at open() time, and the \a logTag to prefix log messages with. > > > > */ > > > > V4L2Device::V4L2Device(const std::string &deviceNode) > > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr), > > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr), > > > > frameStartEnabled_(false) > > > > { > > > > } > > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags) > > > > return ret; > > > > } > > > > > > > > - setFd(ret); > > > > + setFd(ScopedFD(ret)); > > > > > > > > listControls(); > > > > > > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags) > > > > * > > > > * \return 0 on success or a negative error code otherwise > > > > */ > > > > -int V4L2Device::setFd(int fd) > > > > +int V4L2Device::setFd(ScopedFD fd) > > > > { > > > > if (isOpen()) > > > > return -EBUSY; > > > > > > > > - fd_ = fd; > > > > + fd_ = std::move(fd); > > > > > > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception); > > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception); > > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable); > > > > fdEventNotifier_->setEnabled(false); > > > > > > > > @@ -138,10 +138,7 @@ void V4L2Device::close() > > > > > > > > delete fdEventNotifier_; > > > > > > > > - if (::close(fd_) < 0) > > > > - LOG(V4L2, Error) << "Failed to close V4L2 device: " > > > > - << strerror(errno); > > > > - fd_ = -1; > > > > + fd_.reset(); > > > > } > > > > > > > > /** > > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp) > > > > * Printing out an error message is usually better performed > > > > * in the caller, which can provide more context. > > > > */ > > > > - if (::ioctl(fd_, request, argp) < 0) > > > > + if (::ioctl(fd_.get(), request, argp) < 0) > > > > return -errno; > > > > > > > > return 0; > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > index 12c09dc7..0bf3b5f5 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -22,6 +22,7 @@ > > > > #include <linux/version.h> > > > > > > > > #include <libcamera/file_descriptor.h> > > > > +#include <libcamera/scoped_file_descriptor.h> > > > > > > > > #include "libcamera/internal/event_notifier.h" > > > > #include "libcamera/internal/log.h" > > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > > > > return ret; > > > > } > > > > > > > > - ret = V4L2Device::setFd(newFd); > > > > + ret = V4L2Device::setFd(ScopedFD(newFd)); > > > > if (ret < 0) { > > > > LOG(V4L2, Error) << "Failed to set file handle: " > > > > << strerror(-ret);
Hi Laurent, On Mon, Nov 29, 2021 at 12:50 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Replying to an old e-mail, > > On Mon, Jun 28, 2021 at 05:49:56AM +0300, Laurent Pinchart wrote: > > On Mon, Jun 07, 2021 at 12:34:00PM +0900, Hirokazu Honda wrote: > > > On Mon, Jun 7, 2021 at 3:19 AM Laurent Pinchart wrote: > > > > On Thu, Apr 15, 2021 at 05:38:39PM +0900, Hirokazu Honda wrote: > > > > > V4L2Device owns a file descriptor for a v4l2 device node. It > > > > > should be managed by ScopedFD avoid the leakage. > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > --- > > > > > include/libcamera/internal/v4l2_device.h | 9 +++++---- > > > > > src/libcamera/v4l2_device.cpp | 17 +++++++---------- > > > > > src/libcamera/v4l2_videodevice.cpp | 3 ++- > > > > > 3 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > > > index d006bf68..e0262de0 100644 > > > > > --- a/include/libcamera/internal/v4l2_device.h > > > > > +++ b/include/libcamera/internal/v4l2_device.h > > > > > @@ -13,6 +13,7 @@ > > > > > > > > > > #include <linux/videodev2.h> > > > > > > > > > > +#include <libcamera/scoped_file_descriptor.h> > > > > > #include <libcamera/signal.h> > > > > > > > > > > #include "libcamera/internal/log.h" > > > > > @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable > > > > > { > > > > > public: > > > > > void close(); > > > > > - bool isOpen() const { return fd_ != -1; } > > > > > + bool isOpen() const { return fd_.isValid(); } > > > > > > > > > > const ControlInfoMap &controls() const { return controls_; } > > > > > > > > > > @@ -46,11 +47,11 @@ protected: > > > > > ~V4L2Device(); > > > > > > > > > > int open(unsigned int flags); > > > > > - int setFd(int fd); > > > > > + int setFd(ScopedFD fd); > > > > > > > > Should this take an rvalue reference to enable move semantics ? > > > > > > Since ScopedFD is move-only, when declaring ScopedFD, it forces a caller to > > > pass with move semantics. > > > > This means that a new ScopedFD instance will be created using the move > > constructor, to be passed to the setFd() function, which will internally > > use the move assignment operator. We can avoid the move constructor by > > passing an rvalue reference here. It would also clearly indicate to the > > user that a move is the expected behaviour of setFd(), while this is > > currently implicit only, as a consequence of ScopedFD deleting the copy > > constructor. > > It recently came to my attention that passing a std::unique_ptr<> by > rvalue reference is usually a bad idea. The class is cheap to move, and > with a pass-by-value call, it's clear that move is being implemented as > std::unique_ptr<> has no copy constructor. When passing by rvalue > reference, the caller has to use std::move(), but it's up to the callee > to actually perform the move. Compare the following: > > void foo(std::unique_ptr<Foo> ptr) > { > } > > void foo(std::unique_ptr<Foo> &&ptr) > { > } > > with foo being called as > > std::unique_ptr<Foo> f = ...; > foo(std::move(f)); > > In the first case, ptr is constructed by moving f. ptr gets destroyed at > the end of foo(), and because foo() is empty, it still owns the pointer, > which then gets deleted. When foo() returns, f doesn't own a pointer > anymore. > > In the second case, f will still own the pointer after foo() returns. Of > course, if foo() was implemented as > > void foo(std::unique_ptr<Foo> &&ptr) > { > globalPtr = std::move(ptr); > } > > then f would not own the pointer after foo() returns. The point here is > that passing a std::unique_ptr<> rvalue reference makes the behaviour > more dependent on the implementation of the function, while passing a > value ensures that f will not own the pointer anymore when foo() > returns, regardless of how it's implemented. > > The same applies to ScopedFD. > > Of course, if the class was expensive to move, it would be a different > cases. > You're right. I was told the same thing a few years ago by my colleague that an argument must be a value if the function makes sure to take the ownership. Otherwise, a destructor of passed value by std::move() can be called in the end of scope in the caller side. -Hiro > > > I would take a value as if ScopedFD is unique_ptr. > > > > > > > > > > > > > int ioctl(unsigned long request, void *argp); > > > > > > > > > > - int fd() const { return fd_; } > > > > > + int fd() const { return fd_.get(); } > > > > > > > > > > private: > > > > > void listControls(); > > > > > @@ -64,7 +65,7 @@ private: > > > > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > > > > ControlInfoMap controls_; > > > > > std::string deviceNode_; > > > > > - int fd_; > > > > > + ScopedFD fd_; > > > > > > > > > > EventNotifier *fdEventNotifier_; > > > > > bool frameStartEnabled_; > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > > index decd19ef..4fbb2d60 100644 > > > > > --- a/src/libcamera/v4l2_device.cpp > > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > > @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2) > > > > > * at open() time, and the \a logTag to prefix log messages with. > > > > > */ > > > > > V4L2Device::V4L2Device(const std::string &deviceNode) > > > > > - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr), > > > > > + : deviceNode_(deviceNode), fdEventNotifier_(nullptr), > > > > > frameStartEnabled_(false) > > > > > { > > > > > } > > > > > @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags) > > > > > return ret; > > > > > } > > > > > > > > > > - setFd(ret); > > > > > + setFd(ScopedFD(ret)); > > > > > > > > > > listControls(); > > > > > > > > > > @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags) > > > > > * > > > > > * \return 0 on success or a negative error code otherwise > > > > > */ > > > > > -int V4L2Device::setFd(int fd) > > > > > +int V4L2Device::setFd(ScopedFD fd) > > > > > { > > > > > if (isOpen()) > > > > > return -EBUSY; > > > > > > > > > > - fd_ = fd; > > > > > + fd_ = std::move(fd); > > > > > > > > > > - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception); > > > > > + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception); > > > > > fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable); > > > > > fdEventNotifier_->setEnabled(false); > > > > > > > > > > @@ -138,10 +138,7 @@ void V4L2Device::close() > > > > > > > > > > delete fdEventNotifier_; > > > > > > > > > > - if (::close(fd_) < 0) > > > > > - LOG(V4L2, Error) << "Failed to close V4L2 device: " > > > > > - << strerror(errno); > > > > > - fd_ = -1; > > > > > + fd_.reset(); > > > > > } > > > > > > > > > > /** > > > > > @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp) > > > > > * Printing out an error message is usually better performed > > > > > * in the caller, which can provide more context. > > > > > */ > > > > > - if (::ioctl(fd_, request, argp) < 0) > > > > > + if (::ioctl(fd_.get(), request, argp) < 0) > > > > > return -errno; > > > > > > > > > > return 0; > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > index 12c09dc7..0bf3b5f5 100644 > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > @@ -22,6 +22,7 @@ > > > > > #include <linux/version.h> > > > > > > > > > > #include <libcamera/file_descriptor.h> > > > > > +#include <libcamera/scoped_file_descriptor.h> > > > > > > > > > > #include "libcamera/internal/event_notifier.h" > > > > > #include "libcamera/internal/log.h" > > > > > @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) > > > > > return ret; > > > > > } > > > > > > > > > > - ret = V4L2Device::setFd(newFd); > > > > > + ret = V4L2Device::setFd(ScopedFD(newFd)); > > > > > if (ret < 0) { > > > > > LOG(V4L2, Error) << "Failed to set file handle: " > > > > > << strerror(-ret); > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index d006bf68..e0262de0 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -13,6 +13,7 @@ #include <linux/videodev2.h> +#include <libcamera/scoped_file_descriptor.h> #include <libcamera/signal.h> #include "libcamera/internal/log.h" @@ -26,7 +27,7 @@ class V4L2Device : protected Loggable { public: void close(); - bool isOpen() const { return fd_ != -1; } + bool isOpen() const { return fd_.isValid(); } const ControlInfoMap &controls() const { return controls_; } @@ -46,11 +47,11 @@ protected: ~V4L2Device(); int open(unsigned int flags); - int setFd(int fd); + int setFd(ScopedFD fd); int ioctl(unsigned long request, void *argp); - int fd() const { return fd_; } + int fd() const { return fd_.get(); } private: void listControls(); @@ -64,7 +65,7 @@ private: std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; ControlInfoMap controls_; std::string deviceNode_; - int fd_; + ScopedFD fd_; EventNotifier *fdEventNotifier_; bool frameStartEnabled_; diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index decd19ef..4fbb2d60 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -53,7 +53,7 @@ LOG_DEFINE_CATEGORY(V4L2) * at open() time, and the \a logTag to prefix log messages with. */ V4L2Device::V4L2Device(const std::string &deviceNode) - : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr), + : deviceNode_(deviceNode), fdEventNotifier_(nullptr), frameStartEnabled_(false) { } @@ -89,7 +89,7 @@ int V4L2Device::open(unsigned int flags) return ret; } - setFd(ret); + setFd(ScopedFD(ret)); listControls(); @@ -112,14 +112,14 @@ int V4L2Device::open(unsigned int flags) * * \return 0 on success or a negative error code otherwise */ -int V4L2Device::setFd(int fd) +int V4L2Device::setFd(ScopedFD fd) { if (isOpen()) return -EBUSY; - fd_ = fd; + fd_ = std::move(fd); - fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception); + fdEventNotifier_ = new EventNotifier(fd_.get(), EventNotifier::Exception); fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable); fdEventNotifier_->setEnabled(false); @@ -138,10 +138,7 @@ void V4L2Device::close() delete fdEventNotifier_; - if (::close(fd_) < 0) - LOG(V4L2, Error) << "Failed to close V4L2 device: " - << strerror(errno); - fd_ = -1; + fd_.reset(); } /** @@ -449,7 +446,7 @@ int V4L2Device::ioctl(unsigned long request, void *argp) * Printing out an error message is usually better performed * in the caller, which can provide more context. */ - if (::ioctl(fd_, request, argp) < 0) + if (::ioctl(fd_.get(), request, argp) < 0) return -errno; return 0; diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 12c09dc7..0bf3b5f5 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -22,6 +22,7 @@ #include <linux/version.h> #include <libcamera/file_descriptor.h> +#include <libcamera/scoped_file_descriptor.h> #include "libcamera/internal/event_notifier.h" #include "libcamera/internal/log.h" @@ -614,7 +615,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type) return ret; } - ret = V4L2Device::setFd(newFd); + ret = V4L2Device::setFd(ScopedFD(newFd)); if (ret < 0) { LOG(V4L2, Error) << "Failed to set file handle: " << strerror(-ret);
V4L2Device owns a file descriptor for a v4l2 device node. It should be managed by ScopedFD avoid the leakage. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/internal/v4l2_device.h | 9 +++++---- src/libcamera/v4l2_device.cpp | 17 +++++++---------- src/libcamera/v4l2_videodevice.cpp | 3 ++- 3 files changed, 14 insertions(+), 15 deletions(-)