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

[Laurent Pinchart]

Hi Hiro,

On Mon, Nov 29, 2021 at 10:54:12PM +0900, Hirokazu Honda wrote:
> On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart 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?

That's a good question. If we want to handle syscall retries (isn't the
libc support to do that by the way ?), I think we should use the File
class (or a new similar class) and extend it with ioctl support, instead
of adding manual retries everywhere.

> [1] https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=3553913abdd97123c3937277f26cba44e6eacacf
> 
> > +       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