| Message ID | 20211128235752.10836-8-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thank you for the patch. On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > The dup() function returns a duplicate of the file descriptor. Wrapping > it in a FileDescriptor isn't wrong as such, but it prevents from using > it in contexts where a UniqueFD is needed. As the duplicate is > guaranteed to have a single owner when created, return it as a UniqueFD > instead. A FileDescriptor can easily be created from the UniqueFD if > desired. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I think this is a correct change. Although I have no idea when dup() should be used. In fact, there seems to be no code calling FileDescriptor::dup(). Do you have any idea? Thanks, -Hiro > --- > include/libcamera/base/file_descriptor.h | 2 +- > src/libcamera/base/file_descriptor.cpp | 22 ++++++++++++++-------- > 2 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > index 74292eba04f5..12a43f95d414 100644 > --- a/include/libcamera/base/file_descriptor.h > +++ b/include/libcamera/base/file_descriptor.h > @@ -28,7 +28,7 @@ public: > > bool isValid() const { return fd_ != nullptr; } > int fd() const { return fd_ ? fd_->fd() : -1; } > - FileDescriptor dup() const; > + UniqueFD dup() const; > > private: > class Descriptor > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > index da696b2501cd..a83bf52c31e6 100644 > --- a/src/libcamera/base/file_descriptor.cpp > +++ b/src/libcamera/base/file_descriptor.cpp > @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other) > * \brief Duplicate a FileDescriptor > * > * Duplicating a FileDescriptor creates a duplicate of the wrapped file > - * descriptor and returns a new FileDescriptor instance that wraps the > - * duplicate. The fd() function of the original and duplicate instances will > - * return different values. The duplicate instance will not be affected by > - * destruction of the original instance or its copies. > + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function > + * of the original and the get() function of the duplicate will return different > + * values. The duplicate instance will not be affected by destruction of the > + * original instance or its copies. > * > - * \return A new FileDescriptor instance wrapping a duplicate of the original > - * file descriptor > + * \return A UniqueFD owning a duplicate of the original file descriptor > */ > -FileDescriptor FileDescriptor::dup() const > +UniqueFD FileDescriptor::dup() const > { > - return FileDescriptor(fd()); > + int dupFd = ::dup(fd()); > + if (dupFd == -1) { > + int ret = -errno; > + LOG(FileDescriptor, Error) > + << "Failed to dup() fd: " << strerror(-ret); > + } > + > + return UniqueFD(dupFd); > } > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Mon, Nov 29, 2021 at 01:57:42AM +0200, Laurent Pinchart wrote: > The dup() function returns a duplicate of the file descriptor. Wrapping > it in a FileDescriptor isn't wrong as such, but it prevents from using > it in contexts where a UniqueFD is needed. As the duplicate is > guaranteed to have a single owner when created, return it as a UniqueFD > instead. A FileDescriptor can easily be created from the UniqueFD if > desired. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/file_descriptor.h | 2 +- > src/libcamera/base/file_descriptor.cpp | 22 ++++++++++++++-------- > 2 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > index 74292eba04f5..12a43f95d414 100644 > --- a/include/libcamera/base/file_descriptor.h > +++ b/include/libcamera/base/file_descriptor.h > @@ -28,7 +28,7 @@ public: > > bool isValid() const { return fd_ != nullptr; } > int fd() const { return fd_ ? fd_->fd() : -1; } > - FileDescriptor dup() const; > + UniqueFD dup() const; > > private: > class Descriptor > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > index da696b2501cd..a83bf52c31e6 100644 > --- a/src/libcamera/base/file_descriptor.cpp > +++ b/src/libcamera/base/file_descriptor.cpp > @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other) > * \brief Duplicate a FileDescriptor > * > * Duplicating a FileDescriptor creates a duplicate of the wrapped file > - * descriptor and returns a new FileDescriptor instance that wraps the > - * duplicate. The fd() function of the original and duplicate instances will > - * return different values. The duplicate instance will not be affected by > - * destruction of the original instance or its copies. > + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function > + * of the original and the get() function of the duplicate will return different Just noticed that we now have: SharedFD::fd() UniqueFD::get() Should the two be named the same ? > + * values. The duplicate instance will not be affected by destruction of the > + * original instance or its copies. > * > - * \return A new FileDescriptor instance wrapping a duplicate of the original > - * file descriptor > + * \return A UniqueFD owning a duplicate of the original file descriptor > */ > -FileDescriptor FileDescriptor::dup() const > +UniqueFD FileDescriptor::dup() const > { > - return FileDescriptor(fd()); > + int dupFd = ::dup(fd()); > + if (dupFd == -1) { > + int ret = -errno; As we don't need to return ret this can simply be in ret = errno; > + LOG(FileDescriptor, Error) > + << "Failed to dup() fd: " << strerror(-ret); and << strerror(ret); > + } > + > + return UniqueFD(dupFd); Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > } > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Nov 29, 2021 at 04:12:33PM +0100, Jacopo Mondi wrote: > On Mon, Nov 29, 2021 at 01:57:42AM +0200, Laurent Pinchart wrote: > > The dup() function returns a duplicate of the file descriptor. Wrapping > > it in a FileDescriptor isn't wrong as such, but it prevents from using > > it in contexts where a UniqueFD is needed. As the duplicate is > > guaranteed to have a single owner when created, return it as a UniqueFD > > instead. A FileDescriptor can easily be created from the UniqueFD if > > desired. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/file_descriptor.h | 2 +- > > src/libcamera/base/file_descriptor.cpp | 22 ++++++++++++++-------- > > 2 files changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > > index 74292eba04f5..12a43f95d414 100644 > > --- a/include/libcamera/base/file_descriptor.h > > +++ b/include/libcamera/base/file_descriptor.h > > @@ -28,7 +28,7 @@ public: > > > > bool isValid() const { return fd_ != nullptr; } > > int fd() const { return fd_ ? fd_->fd() : -1; } > > - FileDescriptor dup() const; > > + UniqueFD dup() const; > > > > private: > > class Descriptor > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > > index da696b2501cd..a83bf52c31e6 100644 > > --- a/src/libcamera/base/file_descriptor.cpp > > +++ b/src/libcamera/base/file_descriptor.cpp > > @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other) > > * \brief Duplicate a FileDescriptor > > * > > * Duplicating a FileDescriptor creates a duplicate of the wrapped file > > - * descriptor and returns a new FileDescriptor instance that wraps the > > - * duplicate. The fd() function of the original and duplicate instances will > > - * return different values. The duplicate instance will not be affected by > > - * destruction of the original instance or its copies. > > + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function > > + * of the original and the get() function of the duplicate will return different > > Just noticed that we now have: > SharedFD::fd() > UniqueFD::get() > > Should the two be named the same ? I think it would make sense, yes. But that brings a question: which of the two should we use ? :-) > > + * values. The duplicate instance will not be affected by destruction of the > > + * original instance or its copies. > > * > > - * \return A new FileDescriptor instance wrapping a duplicate of the original > > - * file descriptor > > + * \return A UniqueFD owning a duplicate of the original file descriptor > > */ > > -FileDescriptor FileDescriptor::dup() const > > +UniqueFD FileDescriptor::dup() const > > { > > - return FileDescriptor(fd()); > > + int dupFd = ::dup(fd()); > > + if (dupFd == -1) { > > + int ret = -errno; > > As we don't need to return ret this can simply be > in ret = errno; > > > + LOG(FileDescriptor, Error) > > + << "Failed to dup() fd: " << strerror(-ret); > > and > << strerror(ret); Indeed. I'm tempted to keep the current pattern though, to avoid making this a special case, but if you think it would be better, I'll change it. > > + } > > + > > + return UniqueFD(dupFd); > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > } > > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
Hi Laurent, On Mon, Nov 29, 2021 at 06:45:10PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Nov 29, 2021 at 04:12:33PM +0100, Jacopo Mondi wrote: > > On Mon, Nov 29, 2021 at 01:57:42AM +0200, Laurent Pinchart wrote: > > > The dup() function returns a duplicate of the file descriptor. Wrapping > > > it in a FileDescriptor isn't wrong as such, but it prevents from using > > > it in contexts where a UniqueFD is needed. As the duplicate is > > > guaranteed to have a single owner when created, return it as a UniqueFD > > > instead. A FileDescriptor can easily be created from the UniqueFD if > > > desired. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/base/file_descriptor.h | 2 +- > > > src/libcamera/base/file_descriptor.cpp | 22 ++++++++++++++-------- > > > 2 files changed, 15 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > > > index 74292eba04f5..12a43f95d414 100644 > > > --- a/include/libcamera/base/file_descriptor.h > > > +++ b/include/libcamera/base/file_descriptor.h > > > @@ -28,7 +28,7 @@ public: > > > > > > bool isValid() const { return fd_ != nullptr; } > > > int fd() const { return fd_ ? fd_->fd() : -1; } > > > - FileDescriptor dup() const; > > > + UniqueFD dup() const; > > > > > > private: > > > class Descriptor > > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > > > index da696b2501cd..a83bf52c31e6 100644 > > > --- a/src/libcamera/base/file_descriptor.cpp > > > +++ b/src/libcamera/base/file_descriptor.cpp > > > @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other) > > > * \brief Duplicate a FileDescriptor > > > * > > > * Duplicating a FileDescriptor creates a duplicate of the wrapped file > > > - * descriptor and returns a new FileDescriptor instance that wraps the > > > - * duplicate. The fd() function of the original and duplicate instances will > > > - * return different values. The duplicate instance will not be affected by > > > - * destruction of the original instance or its copies. > > > + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function > > > + * of the original and the get() function of the duplicate will return different > > > > Just noticed that we now have: > > SharedFD::fd() > > UniqueFD::get() > > > > Should the two be named the same ? > > I think it would make sense, yes. But that brings a question: which of > the two should we use ? :-) fd.fd() is awful :) Also *_ptr<> have a .get() function, so.... > > > > + * values. The duplicate instance will not be affected by destruction of the > > > + * original instance or its copies. > > > * > > > - * \return A new FileDescriptor instance wrapping a duplicate of the original > > > - * file descriptor > > > + * \return A UniqueFD owning a duplicate of the original file descriptor > > > */ > > > -FileDescriptor FileDescriptor::dup() const > > > +UniqueFD FileDescriptor::dup() const > > > { > > > - return FileDescriptor(fd()); > > > + int dupFd = ::dup(fd()); > > > + if (dupFd == -1) { > > > + int ret = -errno; > > > > As we don't need to return ret this can simply be > > in ret = errno; > > > > > + LOG(FileDescriptor, Error) > > > + << "Failed to dup() fd: " << strerror(-ret); > > > > and > > << strerror(ret); > > Indeed. I'm tempted to keep the current pattern though, to avoid making > this a special case, but if you think it would be better, I'll change > it. > I know, it's a stupid detail, whatever is fine! > > > + } > > > + > > > + return UniqueFD(dupFd); > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > } > > > > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Mon, Nov 29, 2021 at 11:05:51PM +0900, Hirokazu Honda wrote: > On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote: > > > > The dup() function returns a duplicate of the file descriptor. Wrapping > > it in a FileDescriptor isn't wrong as such, but it prevents from using > > it in contexts where a UniqueFD is needed. As the duplicate is > > guaranteed to have a single owner when created, return it as a UniqueFD > > instead. A FileDescriptor can easily be created from the UniqueFD if > > desired. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I think this is a correct change. > Although I have no idea when dup() should be used. > In fact, there seems to be no code calling FileDescriptor::dup(). > Do you have any idea? You're right, it's not used. There's a place where it would be useful though, I'll give it a try in v4. > > --- > > include/libcamera/base/file_descriptor.h | 2 +- > > src/libcamera/base/file_descriptor.cpp | 22 ++++++++++++++-------- > > 2 files changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > > index 74292eba04f5..12a43f95d414 100644 > > --- a/include/libcamera/base/file_descriptor.h > > +++ b/include/libcamera/base/file_descriptor.h > > @@ -28,7 +28,7 @@ public: > > > > bool isValid() const { return fd_ != nullptr; } > > int fd() const { return fd_ ? fd_->fd() : -1; } > > - FileDescriptor dup() const; > > + UniqueFD dup() const; > > > > private: > > class Descriptor > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > > index da696b2501cd..a83bf52c31e6 100644 > > --- a/src/libcamera/base/file_descriptor.cpp > > +++ b/src/libcamera/base/file_descriptor.cpp > > @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other) > > * \brief Duplicate a FileDescriptor > > * > > * Duplicating a FileDescriptor creates a duplicate of the wrapped file > > - * descriptor and returns a new FileDescriptor instance that wraps the > > - * duplicate. The fd() function of the original and duplicate instances will > > - * return different values. The duplicate instance will not be affected by > > - * destruction of the original instance or its copies. > > + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function > > + * of the original and the get() function of the duplicate will return different > > + * values. The duplicate instance will not be affected by destruction of the > > + * original instance or its copies. > > * > > - * \return A new FileDescriptor instance wrapping a duplicate of the original > > - * file descriptor > > + * \return A UniqueFD owning a duplicate of the original file descriptor > > */ > > -FileDescriptor FileDescriptor::dup() const > > +UniqueFD FileDescriptor::dup() const > > { > > - return FileDescriptor(fd()); > > + int dupFd = ::dup(fd()); > > + if (dupFd == -1) { > > + int ret = -errno; > > + LOG(FileDescriptor, Error) > > + << "Failed to dup() fd: " << strerror(-ret); > > + } > > + > > + return UniqueFD(dupFd); > > } > > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
Hi Jacopo, On Mon, Nov 29, 2021 at 06:24:27PM +0100, Jacopo Mondi wrote: > On Mon, Nov 29, 2021 at 06:45:10PM +0200, Laurent Pinchart wrote: > > On Mon, Nov 29, 2021 at 04:12:33PM +0100, Jacopo Mondi wrote: > > > On Mon, Nov 29, 2021 at 01:57:42AM +0200, Laurent Pinchart wrote: > > > > The dup() function returns a duplicate of the file descriptor. Wrapping > > > > it in a FileDescriptor isn't wrong as such, but it prevents from using > > > > it in contexts where a UniqueFD is needed. As the duplicate is > > > > guaranteed to have a single owner when created, return it as a UniqueFD > > > > instead. A FileDescriptor can easily be created from the UniqueFD if > > > > desired. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > include/libcamera/base/file_descriptor.h | 2 +- > > > > src/libcamera/base/file_descriptor.cpp | 22 ++++++++++++++-------- > > > > 2 files changed, 15 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > > > > index 74292eba04f5..12a43f95d414 100644 > > > > --- a/include/libcamera/base/file_descriptor.h > > > > +++ b/include/libcamera/base/file_descriptor.h > > > > @@ -28,7 +28,7 @@ public: > > > > > > > > bool isValid() const { return fd_ != nullptr; } > > > > int fd() const { return fd_ ? fd_->fd() : -1; } > > > > - FileDescriptor dup() const; > > > > + UniqueFD dup() const; > > > > > > > > private: > > > > class Descriptor > > > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > > > > index da696b2501cd..a83bf52c31e6 100644 > > > > --- a/src/libcamera/base/file_descriptor.cpp > > > > +++ b/src/libcamera/base/file_descriptor.cpp > > > > @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other) > > > > * \brief Duplicate a FileDescriptor > > > > * > > > > * Duplicating a FileDescriptor creates a duplicate of the wrapped file > > > > - * descriptor and returns a new FileDescriptor instance that wraps the > > > > - * duplicate. The fd() function of the original and duplicate instances will > > > > - * return different values. The duplicate instance will not be affected by > > > > - * destruction of the original instance or its copies. > > > > + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function > > > > + * of the original and the get() function of the duplicate will return different > > > > > > Just noticed that we now have: > > > SharedFD::fd() > > > UniqueFD::get() > > > > > > Should the two be named the same ? > > > > I think it would make sense, yes. But that brings a question: which of > > the two should we use ? :-) > > fd.fd() is awful :) > Also *_ptr<> have a .get() function, so.... I usually prefer explicit names, but in this case I agree that get() is likely better. I'll rename fd() to get(). > > > > + * values. The duplicate instance will not be affected by destruction of the > > > > + * original instance or its copies. > > > > * > > > > - * \return A new FileDescriptor instance wrapping a duplicate of the original > > > > - * file descriptor > > > > + * \return A UniqueFD owning a duplicate of the original file descriptor > > > > */ > > > > -FileDescriptor FileDescriptor::dup() const > > > > +UniqueFD FileDescriptor::dup() const > > > > { > > > > - return FileDescriptor(fd()); > > > > + int dupFd = ::dup(fd()); > > > > + if (dupFd == -1) { > > > > + int ret = -errno; > > > > > > As we don't need to return ret this can simply be > > > in ret = errno; > > > > > > > + LOG(FileDescriptor, Error) > > > > + << "Failed to dup() fd: " << strerror(-ret); > > > > > > and > > > << strerror(ret); > > > > Indeed. I'm tempted to keep the current pattern though, to avoid making > > this a special case, but if you think it would be better, I'll change > > it. > > > > I know, it's a stupid detail, whatever is fine! > > > > > + } > > > > + > > > > + return UniqueFD(dupFd); > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > } > > > > > > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h index 74292eba04f5..12a43f95d414 100644 --- a/include/libcamera/base/file_descriptor.h +++ b/include/libcamera/base/file_descriptor.h @@ -28,7 +28,7 @@ public: bool isValid() const { return fd_ != nullptr; } int fd() const { return fd_ ? fd_->fd() : -1; } - FileDescriptor dup() const; + UniqueFD dup() const; private: class Descriptor diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp index da696b2501cd..a83bf52c31e6 100644 --- a/src/libcamera/base/file_descriptor.cpp +++ b/src/libcamera/base/file_descriptor.cpp @@ -222,17 +222,23 @@ FileDescriptor &FileDescriptor::operator=(FileDescriptor &&other) * \brief Duplicate a FileDescriptor * * Duplicating a FileDescriptor creates a duplicate of the wrapped file - * descriptor and returns a new FileDescriptor instance that wraps the - * duplicate. The fd() function of the original and duplicate instances will - * return different values. The duplicate instance will not be affected by - * destruction of the original instance or its copies. + * descriptor and returns a UniqueFD that owns the duplicate. The fd() function + * of the original and the get() function of the duplicate will return different + * values. The duplicate instance will not be affected by destruction of the + * original instance or its copies. * - * \return A new FileDescriptor instance wrapping a duplicate of the original - * file descriptor + * \return A UniqueFD owning a duplicate of the original file descriptor */ -FileDescriptor FileDescriptor::dup() const +UniqueFD FileDescriptor::dup() const { - return FileDescriptor(fd()); + int dupFd = ::dup(fd()); + if (dupFd == -1) { + int ret = -errno; + LOG(FileDescriptor, Error) + << "Failed to dup() fd: " << strerror(-ret); + } + + return UniqueFD(dupFd); } FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)
The dup() function returns a duplicate of the file descriptor. Wrapping it in a FileDescriptor isn't wrong as such, but it prevents from using it in contexts where a UniqueFD is needed. As the duplicate is guaranteed to have a single owner when created, return it as a UniqueFD instead. A FileDescriptor can easily be created from the UniqueFD if desired. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/base/file_descriptor.h | 2 +- src/libcamera/base/file_descriptor.cpp | 22 ++++++++++++++-------- 2 files changed, 15 insertions(+), 9 deletions(-)