[libcamera-devel,v3,09/17] libcamera: file: Manage fd by UniqueFD
diff mbox series

Message ID 20211128235752.10836-10-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 the file descriptor owned by File by UniqueFD.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/base/file.h |  5 +++--
 src/libcamera/base/file.cpp   | 25 ++++++++++++-------------
 2 files changed, 15 insertions(+), 15 deletions(-)

Comments

Hirokazu Honda Nov. 29, 2021, 1:54 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 the file descriptor owned by File by UniqueFD.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
>
Jacopo Mondi Nov. 29, 2021, 3:22 p.m. UTC | #2 
Hi Laurent

On Mon, Nov 29, 2021 at 01:57:44AM +0200, Laurent Pinchart wrote:
> From: Hirokazu Honda <hiroh@chromium.org>
>
> Manages the file descriptor owned by File by UniqueFD.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

Thanks
  j

> ---
>  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));
> +	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
>
Laurent Pinchart Nov. 29, 2021, 4:52 p.m. UTC | #3 
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@chromium.org>
> >
> > Manages the file descriptor owned by File by UniqueFD.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 {};
Hirokazu Honda Nov. 30, 2021, 1:01 a.m. UTC | #4 
Hi Laurent,

On Tue, Nov 30, 2021 at 1:52 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@chromium.org>
> > >
> > > Manages the file descriptor owned by File by UniqueFD.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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)).

-Hiro
> > [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
Laurent Pinchart Nov. 30, 2021, 1:48 a.m. UTC | #5 
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@chromium.org>
> > > >
> > > > Manages the file descriptor owned by File by UniqueFD.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 {};
Hirokazu Honda Nov. 30, 2021, 2:04 a.m. UTC | #6 
Hi Laurent,

On Tue, Nov 30, 2021 at 10:48 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@chromium.org>
> > > > >
> > > > > Manages the file descriptor owned by File by UniqueFD.
> > > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
>

Ack.

> > > > [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

Patch
diff mbox series 

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));
+	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 {};