[libcamera-devel,v3,13/17] libcamera: v4l2_device: Use UniqueFD for a file descriptor
diff mbox series

Message ID 20211128235752.10836-14-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Introduce UniqueFD
Related show

Commit Message

Laurent Pinchart Nov. 28, 2021, 11:57 p.m. UTC 
From: Hirokazu Honda <hiroh@chromium.org>

Manages a file descriptor owned by V4L2Device for a v4l2 device node
by UniqueFD.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Fix errno handling
---
 include/libcamera/internal/v4l2_device.h |  9 +++++----
 src/libcamera/v4l2_device.cpp            | 23 ++++++++++-------------
 src/libcamera/v4l2_videodevice.cpp       | 16 ++++++----------
 3 files changed, 21 insertions(+), 27 deletions(-)

Comments

Hirokazu Honda Nov. 29, 2021, 1:58 p.m. UTC | #1 
Hi Laurent,

On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> From: Hirokazu Honda <hiroh@chromium.org>
>
> Manages a file descriptor owned by V4L2Device for a v4l2 device node
> by UniqueFD.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org> if applicable.
> ---
> Changes since v2:
>
> - Fix errno handling
> ---
>  include/libcamera/internal/v4l2_device.h |  9 +++++----
>  src/libcamera/v4l2_device.cpp            | 23 ++++++++++-------------
>  src/libcamera/v4l2_videodevice.cpp       | 16 ++++++----------
>  3 files changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 7816a290141d..8886b750ae29 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -16,6 +16,7 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/signal.h>
>  #include <libcamera/base/span.h>
> +#include <libcamera/base/unique_fd.h>
>
>  #include <libcamera/controls.h>
>
> @@ -27,7 +28,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_; }
>
> @@ -49,11 +50,11 @@ protected:
>         ~V4L2Device();
>
>         int open(unsigned int flags);
> -       int setFd(int fd);
> +       int setFd(UniqueFD fd);
>
>         int ioctl(unsigned long request, void *argp);
>
> -       int fd() const { return fd_; }
> +       int fd() const { return fd_.get(); }
>
>  private:
>         static ControlType v4l2CtrlType(uint32_t ctrlType);
> @@ -72,7 +73,7 @@ private:
>         ControlIdMap controlIdMap_;
>         ControlInfoMap controls_;
>         std::string deviceNode_;
> -       int fd_;
> +       UniqueFD fd_;
>
>         EventNotifier *fdEventNotifier_;
>         bool frameStartEnabled_;
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 9c783c9cbed1..39f360091f64 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)
>  {
>  }
> @@ -81,15 +81,15 @@ int V4L2Device::open(unsigned int flags)
>                 return -EBUSY;
>         }
>
> -       int ret = syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags);
> -       if (ret < 0) {
> -               ret = -errno;
> +       UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));
> +       if (!fd.isValid()) {
> +               int ret = -errno;
>                 LOG(V4L2, Error) << "Failed to open V4L2 device: "
>                                  << strerror(-ret);
>                 return ret;
>         }
>
> -       setFd(ret);
> +       setFd(std::move(fd));
>
>         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(UniqueFD 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();
>  }
>
>  /**
> @@ -440,7 +437,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 0a85bcf6b3ff..c95626d33cc3 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -24,6 +24,7 @@
>  #include <libcamera/base/event_notifier.h>
>  #include <libcamera/base/file_descriptor.h>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/unique_fd.h>
>  #include <libcamera/base/utils.h>
>
>  #include "libcamera/internal/formats.h"
> @@ -620,22 +621,17 @@ int V4L2VideoDevice::open()
>   */
>  int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>  {
> -       int ret;
> -       int newFd;
> -
> -       newFd = dup(handle);
> -       if (newFd < 0) {
> -               ret = -errno;
> +       UniqueFD newFd(dup(handle));
> +       if (!newFd.isValid()) {
>                 LOG(V4L2, Error) << "Failed to duplicate file handle: "
> -                                << strerror(-ret);
> -               return ret;
> +                                << strerror(errno);
> +               return -errno;
>         }
>
> -       ret = V4L2Device::setFd(newFd);
> +       int ret = V4L2Device::setFd(std::move(newFd));
>         if (ret < 0) {
>                 LOG(V4L2, Error) << "Failed to set file handle: "
>                                  << strerror(-ret);
> -               ::close(newFd);
>                 return ret;
>         }
>
> --
> Regards,
>
> Laurent Pinchart
>
Jacopo Mondi Nov. 29, 2021, 3:51 p.m. UTC | #2 
Hi Laurent,

On Mon, Nov 29, 2021 at 01:57:48AM +0200, Laurent Pinchart wrote:
> From: Hirokazu Honda <hiroh@chromium.org>
>
> Manages a file descriptor owned by V4L2Device for a v4l2 device node
> by UniqueFD.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
> Changes since v2:
>
> - Fix errno handling
> ---
>  include/libcamera/internal/v4l2_device.h |  9 +++++----
>  src/libcamera/v4l2_device.cpp            | 23 ++++++++++-------------
>  src/libcamera/v4l2_videodevice.cpp       | 16 ++++++----------
>  3 files changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 7816a290141d..8886b750ae29 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -16,6 +16,7 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/signal.h>
>  #include <libcamera/base/span.h>
> +#include <libcamera/base/unique_fd.h>
>
>  #include <libcamera/controls.h>
>
> @@ -27,7 +28,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_; }
>
> @@ -49,11 +50,11 @@ protected:
>  	~V4L2Device();
>
>  	int open(unsigned int flags);
> -	int setFd(int fd);
> +	int setFd(UniqueFD fd);
>
>  	int ioctl(unsigned long request, void *argp);
>
> -	int fd() const { return fd_; }
> +	int fd() const { return fd_.get(); }
>
>  private:
>  	static ControlType v4l2CtrlType(uint32_t ctrlType);
> @@ -72,7 +73,7 @@ private:
>  	ControlIdMap controlIdMap_;
>  	ControlInfoMap controls_;
>  	std::string deviceNode_;
> -	int fd_;
> +	UniqueFD fd_;
>
>  	EventNotifier *fdEventNotifier_;
>  	bool frameStartEnabled_;
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 9c783c9cbed1..39f360091f64 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)
>  {
>  }
> @@ -81,15 +81,15 @@ int V4L2Device::open(unsigned int flags)
>  		return -EBUSY;
>  	}
>
> -	int ret = syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags);
> -	if (ret < 0) {
> -		ret = -errno;
> +	UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));
> +	if (!fd.isValid()) {
> +		int ret = -errno;
>  		LOG(V4L2, Error) << "Failed to open V4L2 device: "
>  				 << strerror(-ret);
>  		return ret;
>  	}
>
> -	setFd(ret);
> +	setFd(std::move(fd));
>
>  	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(UniqueFD 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();
>  }
>
>  /**
> @@ -440,7 +437,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 0a85bcf6b3ff..c95626d33cc3 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -24,6 +24,7 @@
>  #include <libcamera/base/event_notifier.h>
>  #include <libcamera/base/file_descriptor.h>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/unique_fd.h>
>  #include <libcamera/base/utils.h>
>
>  #include "libcamera/internal/formats.h"
> @@ -620,22 +621,17 @@ int V4L2VideoDevice::open()
>   */
>  int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>  {
> -	int ret;
> -	int newFd;
> -
> -	newFd = dup(handle);
> -	if (newFd < 0) {
> -		ret = -errno;
> +	UniqueFD newFd(dup(handle));
> +	if (!newFd.isValid()) {
>  		LOG(V4L2, Error) << "Failed to duplicate file handle: "
> -				 << strerror(-ret);
> -		return ret;
> +				 << strerror(errno);
> +		return -errno;
>  	}
>
> -	ret = V4L2Device::setFd(newFd);
> +	int ret = V4L2Device::setFd(std::move(newFd));
>  	if (ret < 0) {
>  		LOG(V4L2, Error) << "Failed to set file handle: "
>  				 << strerror(-ret);
> -		::close(newFd);
>  		return ret;
>  	}
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
index 7816a290141d..8886b750ae29 100644
--- a/include/libcamera/internal/v4l2_device.h
+++ b/include/libcamera/internal/v4l2_device.h
@@ -16,6 +16,7 @@ 
 #include <libcamera/base/log.h>
 #include <libcamera/base/signal.h>
 #include <libcamera/base/span.h>
+#include <libcamera/base/unique_fd.h>
 
 #include <libcamera/controls.h>
 
@@ -27,7 +28,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_; }
 
@@ -49,11 +50,11 @@  protected:
 	~V4L2Device();
 
 	int open(unsigned int flags);
-	int setFd(int fd);
+	int setFd(UniqueFD fd);
 
 	int ioctl(unsigned long request, void *argp);
 
-	int fd() const { return fd_; }
+	int fd() const { return fd_.get(); }
 
 private:
 	static ControlType v4l2CtrlType(uint32_t ctrlType);
@@ -72,7 +73,7 @@  private:
 	ControlIdMap controlIdMap_;
 	ControlInfoMap controls_;
 	std::string deviceNode_;
-	int fd_;
+	UniqueFD fd_;
 
 	EventNotifier *fdEventNotifier_;
 	bool frameStartEnabled_;
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 9c783c9cbed1..39f360091f64 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)
 {
 }
@@ -81,15 +81,15 @@  int V4L2Device::open(unsigned int flags)
 		return -EBUSY;
 	}
 
-	int ret = syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags);
-	if (ret < 0) {
-		ret = -errno;
+	UniqueFD fd(syscall(SYS_openat, AT_FDCWD, deviceNode_.c_str(), flags));
+	if (!fd.isValid()) {
+		int ret = -errno;
 		LOG(V4L2, Error) << "Failed to open V4L2 device: "
 				 << strerror(-ret);
 		return ret;
 	}
 
-	setFd(ret);
+	setFd(std::move(fd));
 
 	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(UniqueFD 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();
 }
 
 /**
@@ -440,7 +437,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 0a85bcf6b3ff..c95626d33cc3 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -24,6 +24,7 @@ 
 #include <libcamera/base/event_notifier.h>
 #include <libcamera/base/file_descriptor.h>
 #include <libcamera/base/log.h>
+#include <libcamera/base/unique_fd.h>
 #include <libcamera/base/utils.h>
 
 #include "libcamera/internal/formats.h"
@@ -620,22 +621,17 @@  int V4L2VideoDevice::open()
  */
 int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
 {
-	int ret;
-	int newFd;
-
-	newFd = dup(handle);
-	if (newFd < 0) {
-		ret = -errno;
+	UniqueFD newFd(dup(handle));
+	if (!newFd.isValid()) {
 		LOG(V4L2, Error) << "Failed to duplicate file handle: "
-				 << strerror(-ret);
-		return ret;
+				 << strerror(errno);
+		return -errno;
 	}
 
-	ret = V4L2Device::setFd(newFd);
+	int ret = V4L2Device::setFd(std::move(newFd));
 	if (ret < 0) {
 		LOG(V4L2, Error) << "Failed to set file handle: "
 				 << strerror(-ret);
-		::close(newFd);
 		return ret;
 	}