[libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd by UniqueFD
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 30 02:48:26 CET 2021
Hi Hiro,
On Tue, Nov 30, 2021 at 10:01:01AM +0900, Hirokazu Honda wrote:
> On Tue, Nov 30, 2021 at 1:52 AM Laurent Pinchart wrote:
> > 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.
>
> IMO, just like UniqueFD(HANDLE_EINTR(dup(fd))) is not so harmful,
> compared to UniqueFD(dup(d)).
Adding HANDLE_EINTR() manually everywhere is the best way to make sure
it will be forgotten somewhere. I don't like that.
> > > [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
More information about the libcamera-devel
mailing list