[libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd by UniqueFD

[Hirokazu Honda]

Hi Laurent,

On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> From: Hirokazu Honda <hiroh at chromium.org>
>
> Manages the file descriptor owned by File by UniqueFD.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/base/file.h |  5 +++--
>  src/libcamera/base/file.cpp   | 25 ++++++++++++-------------
>  2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h
> index 996751a7ab72..47769da7abc2 100644
> --- a/include/libcamera/base/file.h
> +++ b/include/libcamera/base/file.h
> @@ -17,6 +17,7 @@
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/flags.h>
>  #include <libcamera/base/span.h>
> +#include <libcamera/base/unique_fd.h>
>
>  namespace libcamera {
>
> @@ -50,7 +51,7 @@ public:
>         bool exists() const;
>
>         bool open(OpenMode mode);
> -       bool isOpen() const { return fd_ != -1; }
> +       bool isOpen() const { return fd_.isValid(); }
>         OpenMode openMode() const { return mode_; }
>         void close();
>
> @@ -76,7 +77,7 @@ private:
>         void unmapAll();
>
>         std::string name_;
> -       int fd_;
> +       UniqueFD fd_;
>         OpenMode mode_;
>
>         int error_;
> diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp
> index 7043f9461cf7..66c73c406198 100644
> --- a/src/libcamera/base/file.cpp
> +++ b/src/libcamera/base/file.cpp
> @@ -84,7 +84,7 @@ LOG_DEFINE_CATEGORY(File)
>   * before performing I/O operations.
>   */
>  File::File(const std::string &name)
> -       : name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)
> +       : name_(name), mode_(OpenModeFlag::NotOpen), error_(0)
>  {
>  }
>
> @@ -95,7 +95,7 @@ File::File(const std::string &name)
>   * setFileName().
>   */
>  File::File()
> -       : fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)
> +       : mode_(OpenModeFlag::NotOpen), error_(0)
>  {
>  }
>
> @@ -178,8 +178,8 @@ bool File::open(File::OpenMode mode)
>         if (mode & OpenModeFlag::WriteOnly)
>                 flags |= O_CREAT;
>
> -       fd_ = ::open(name_.c_str(), flags, 0666);
> -       if (fd_ < 0) {
> +       fd_ = UniqueFD(::open(name_.c_str(), flags, 0666));

This is not quite related to this patch series.
Chrome uses HANDLE_EINTR macro [1] to retry system call in the case of EINTR.
It is often used with dup and open call. Shall we use it in libcamera?

[1] https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=3553913abdd97123c3937277f26cba44e6eacacf

-Hiro

> +       if (!fd_.isValid()) {
>                 error_ = -errno;
>                 return false;
>         }
> @@ -210,11 +210,10 @@ bool File::open(File::OpenMode mode)
>   */
>  void File::close()
>  {
> -       if (fd_ == -1)
> +       if (!fd_.isValid())
>                 return;
>
> -       ::close(fd_);
> -       fd_ = -1;
> +       fd_.reset();
>         mode_ = OpenModeFlag::NotOpen;
>  }
>
> @@ -244,7 +243,7 @@ ssize_t File::size() const
>                 return -EINVAL;
>
>         struct stat st;
> -       int ret = fstat(fd_, &st);
> +       int ret = fstat(fd_.get(), &st);
>         if (ret < 0)
>                 return -errno;
>
> @@ -263,7 +262,7 @@ off_t File::pos() const
>         if (!isOpen())
>                 return 0;
>
> -       return lseek(fd_, 0, SEEK_CUR);
> +       return lseek(fd_.get(), 0, SEEK_CUR);
>  }
>
>  /**
> @@ -277,7 +276,7 @@ off_t File::seek(off_t pos)
>         if (!isOpen())
>                 return -EINVAL;
>
> -       off_t ret = lseek(fd_, pos, SEEK_SET);
> +       off_t ret = lseek(fd_.get(), pos, SEEK_SET);
>         if (ret < 0)
>                 return -errno;
>
> @@ -309,7 +308,7 @@ ssize_t File::read(const Span<uint8_t> &data)
>
>         /* Retry in case of interrupted system calls. */
>         while (readBytes < data.size()) {
> -               ret = ::read(fd_, data.data() + readBytes,
> +               ret = ::read(fd_.get(), data.data() + readBytes,
>                              data.size() - readBytes);
>                 if (ret <= 0)
>                         break;
> @@ -346,7 +345,7 @@ ssize_t File::write(const Span<const uint8_t> &data)
>
>         /* Retry in case of interrupted system calls. */
>         while (writtenBytes < data.size()) {
> -               ssize_t ret = ::write(fd_, data.data() + writtenBytes,
> +               ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,
>                                       data.size() - writtenBytes);
>                 if (ret <= 0)
>                         break;
> @@ -409,7 +408,7 @@ Span<uint8_t> File::map(off_t offset, ssize_t size, File::MapFlags flags)
>         if (flags & MapFlag::Private)
>                 prot |= PROT_WRITE;
>
> -       void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);
> +       void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);
>         if (map == MAP_FAILED) {
>                 error_ = -errno;
>                 return {};
> --
> Regards,
>
> Laurent Pinchart
>