| Message ID | 20191126233620.1695316-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Wed, Nov 27, 2019 at 12:35:54AM +0100, Niklas Söderlund wrote: > Add a helper to make it easier to pass file descriptors around. The > helper class duplicates the fd which decouples it from the original fd > which could be closed by its owner while the new FileDescriptor remains > valid. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/buffer.h | 12 ++++++++++++ > src/libcamera/buffer.cpp | 40 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 7302b9f32ce8c50d..33793b4ccf881eda 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -46,6 +46,18 @@ private: > std::vector<Plane> planes_; > }; > > +class FileDescriptor final > +{ > +public: > + FileDescriptor(int fd); > + ~FileDescriptor(); > + > + int fd() const { return fd_; } > + > +private: > + int fd_; > +}; > + > class Plane final > { > public: > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index 4acff216cafba484..0676586ae3be2a61 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -115,6 +115,46 @@ void BufferInfo::update(Status status, unsigned int sequence, > * \return A array holding all plane information for the buffer > */ > > +/** > + * \class FileDescriptor > + * \brief Life time management of a file descriptor > + * > + * The managed file descriptor (fd) is duplicated from the original one and > + * is opened for the life time of the FileDescriptor object disregarding s/is opened/stays opened/ ? > + * if the original one is closed. > + */ > + > +/** > + * \brief Create a life time managed file descriptor > + * \param[in] fd File descriptor > + */ > +FileDescriptor::FileDescriptor(int fd) > + : fd_(-1) > +{ > + if (fd < 0) > + return; If one has gone so fare as providing an invalid fd to the constructor, probably has missed some error check, I would LOG() an error here (or even Fatal ? ) > + > + /* Failing to dup() a fd should not happen and is fatal. */ > + fd_ = dup(fd); > + if (fd_ == -1) { > + int ret = -errno; > + LOG(Buffer, Fatal) > + << "Failed to dup() fd: " << strerror(-ret); > + } > +} > + > +FileDescriptor::~FileDescriptor() > +{ > + if (fd_ != -1) > + close(fd_); > +} > + > +/** > + * \fn FileDescriptor::fd() > + * \brief Retrieve the file descriptor or or.. ? > + * \return A valid file descriptor or a negative error code Not a negative error code, but -1 if the instance has been created with an invalid file descriptor. How to put it nicely here, well... not sure :) Wording apart: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + */ > + > /** > * \class Plane > * \brief A memory region to store a single plane of a frame > -- > 2.24.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Wed, Nov 27, 2019 at 11:52:04AM +0100, Jacopo Mondi wrote: > On Wed, Nov 27, 2019 at 12:35:54AM +0100, Niklas Söderlund wrote: > > Add a helper to make it easier to pass file descriptors around. The > > helper class duplicates the fd which decouples it from the original fd > > which could be closed by its owner while the new FileDescriptor remains > > valid. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/buffer.h | 12 ++++++++++++ > > src/libcamera/buffer.cpp | 40 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 52 insertions(+) > > > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > > index 7302b9f32ce8c50d..33793b4ccf881eda 100644 > > --- a/include/libcamera/buffer.h > > +++ b/include/libcamera/buffer.h > > @@ -46,6 +46,18 @@ private: > > std::vector<Plane> planes_; > > }; > > > > +class FileDescriptor final > > +{ > > +public: > > + FileDescriptor(int fd); > > + ~FileDescriptor(); You're missing the copy constructor and assignment operator that should dup() the fd, and the move versions that should set them to -1 in the original. All constructors should be marked as explicit to avoid unwanted copies, as those are a bit expensive. > > + > > + int fd() const { return fd_; } > > + > > +private: > > + int fd_; > > +}; > > + > > class Plane final > > { > > public: > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > > index 4acff216cafba484..0676586ae3be2a61 100644 > > --- a/src/libcamera/buffer.cpp > > +++ b/src/libcamera/buffer.cpp > > @@ -115,6 +115,46 @@ void BufferInfo::update(Status status, unsigned int sequence, > > * \return A array holding all plane information for the buffer > > */ > > > > +/** > > + * \class FileDescriptor > > + * \brief Life time management of a file descriptor > > + * > > + * The managed file descriptor (fd) is duplicated from the original one and > > + * is opened for the life time of the FileDescriptor object disregarding > > s/is opened/stays opened/ ? > > > + * if the original one is closed. * \brief RAII-style wrapper for file descriptors * * The FileDescriptor class wraps a file descriptor (expressed as a signed * integer) to provide an RAII-style mechanism for owning the file descriptor. * When constructed, the FileDescriptor instance duplicates a given file * descriptor and takes ownership of the duplicate as a resource. The copy * constructor and assignment operator duplicate the file descriptor, while the * move version of those methods move the resource and make the original * FileDescriptor invalid. When the FileDescriptor is deleted, it closes the * file descriptor it owns, if any. (See https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization for a definition of RAII) > > + */ > > + > > +/** > > + * \brief Create a life time managed file descriptor > > + * \param[in] fd File descriptor * \brief Create a FileDescriptor wrapping a copy of a given \a fd * \param[in] fd File descriptor * * Constructing a FileDescriptor from a numerical file descriptor duplicates the * \a fd and takes ownership of the copy. The original \a fd is left untouched, * and the caller is responsible for closing it when appropriate. The duplicated * file descriptor will be closed automatically when the FileDescriptor instance * is destroyed. I wonder if we should also define a FileDescriptor::FileDescriptor(int &&fd) that skips the duplication, as the caller may sometimes not have a need to keep the original around. > > + */ > > +FileDescriptor::FileDescriptor(int fd) > > + : fd_(-1) > > +{ > > + if (fd < 0) > > + return; > > If one has gone so fare as providing an invalid fd to the constructor, > probably has missed some error check, I would LOG() an error here (or > even Fatal ? ) There could be use cases for creating FileDescriptor instances that don't own any file descriptor, and then assigning a valid instance to them with operator=(). I would this not add any check here, and set a default value of -1 for the fd parameter in the header file. > > + > > + /* Failing to dup() a fd should not happen and is fatal. */ > > + fd_ = dup(fd); > > + if (fd_ == -1) { > > + int ret = -errno; > > + LOG(Buffer, Fatal) > > + << "Failed to dup() fd: " << strerror(-ret); > > + } > > +} > > + > > +FileDescriptor::~FileDescriptor() > > +{ > > + if (fd_ != -1) > > + close(fd_); > > +} > > + > > +/** > > + * \fn FileDescriptor::fd() > > + * \brief Retrieve the file descriptor or > > or.. ? > > > + * \return A valid file descriptor or a negative error code > > Not a negative error code, but -1 if the instance has been created > with an invalid file descriptor. How to put it nicely here, well... > not sure :) * \brief Retrieve the numerical file descriptor * \return The numerical file descriptor, which may be -1 if the FileDescriptor * instance doesn't own a file descriptor > > Wording apart: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + */ > > + > > /** > > * \class Plane > > * \brief A memory region to store a single plane of a frame
Hello Niklas, On Wed, Nov 27, 2019 at 12:35:54AM +0100, Niklas Söderlund wrote: > Add a helper to make it easier to pass file descriptors around. The > helper class duplicates the fd which decouples it from the original fd > which could be closed by its owner while the new FileDescriptor remains > valid. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/buffer.h | 12 ++++++++++++ > src/libcamera/buffer.cpp | 40 ++++++++++++++++++++++++++++++++++++++ I missed this, you should move this class to file_descriptor.{h,cpp} as it may be useful beyond buffers. > 2 files changed, 52 insertions(+) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 7302b9f32ce8c50d..33793b4ccf881eda 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -46,6 +46,18 @@ private: > std::vector<Plane> planes_; > }; > > +class FileDescriptor final > +{ > +public: > + FileDescriptor(int fd); > + ~FileDescriptor(); > + > + int fd() const { return fd_; } > + > +private: > + int fd_; > +}; > + > class Plane final > { > public: > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index 4acff216cafba484..0676586ae3be2a61 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -115,6 +115,46 @@ void BufferInfo::update(Status status, unsigned int sequence, > * \return A array holding all plane information for the buffer > */ > > +/** > + * \class FileDescriptor > + * \brief Life time management of a file descriptor > + * > + * The managed file descriptor (fd) is duplicated from the original one and > + * is opened for the life time of the FileDescriptor object disregarding > + * if the original one is closed. > + */ > + > +/** > + * \brief Create a life time managed file descriptor > + * \param[in] fd File descriptor > + */ > +FileDescriptor::FileDescriptor(int fd) > + : fd_(-1) > +{ > + if (fd < 0) > + return; > + > + /* Failing to dup() a fd should not happen and is fatal. */ > + fd_ = dup(fd); > + if (fd_ == -1) { > + int ret = -errno; > + LOG(Buffer, Fatal) > + << "Failed to dup() fd: " << strerror(-ret); > + } > +} > + > +FileDescriptor::~FileDescriptor() > +{ > + if (fd_ != -1) > + close(fd_); > +} > + > +/** > + * \fn FileDescriptor::fd() > + * \brief Retrieve the file descriptor or > + * \return A valid file descriptor or a negative error code > + */ > + > /** > * \class Plane > * \brief A memory region to store a single plane of a frame
diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h index 7302b9f32ce8c50d..33793b4ccf881eda 100644 --- a/include/libcamera/buffer.h +++ b/include/libcamera/buffer.h @@ -46,6 +46,18 @@ private: std::vector<Plane> planes_; }; +class FileDescriptor final +{ +public: + FileDescriptor(int fd); + ~FileDescriptor(); + + int fd() const { return fd_; } + +private: + int fd_; +}; + class Plane final { public: diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp index 4acff216cafba484..0676586ae3be2a61 100644 --- a/src/libcamera/buffer.cpp +++ b/src/libcamera/buffer.cpp @@ -115,6 +115,46 @@ void BufferInfo::update(Status status, unsigned int sequence, * \return A array holding all plane information for the buffer */ +/** + * \class FileDescriptor + * \brief Life time management of a file descriptor + * + * The managed file descriptor (fd) is duplicated from the original one and + * is opened for the life time of the FileDescriptor object disregarding + * if the original one is closed. + */ + +/** + * \brief Create a life time managed file descriptor + * \param[in] fd File descriptor + */ +FileDescriptor::FileDescriptor(int fd) + : fd_(-1) +{ + if (fd < 0) + return; + + /* Failing to dup() a fd should not happen and is fatal. */ + fd_ = dup(fd); + if (fd_ == -1) { + int ret = -errno; + LOG(Buffer, Fatal) + << "Failed to dup() fd: " << strerror(-ret); + } +} + +FileDescriptor::~FileDescriptor() +{ + if (fd_ != -1) + close(fd_); +} + +/** + * \fn FileDescriptor::fd() + * \brief Retrieve the file descriptor or + * \return A valid file descriptor or a negative error code + */ + /** * \class Plane * \brief A memory region to store a single plane of a frame
Add a helper to make it easier to pass file descriptors around. The helper class duplicates the fd which decouples it from the original fd which could be closed by its owner while the new FileDescriptor remains valid. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/buffer.h | 12 ++++++++++++ src/libcamera/buffer.cpp | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+)