| Message ID | 20211128235752.10836-4-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 inode() function has always been a bit of an outcast in the > FileDescriptor class, as it's not related to the core feature provided > by FileDescriptor, a shared ownership wrapper around file descriptors. > Move it to the File class instead. It may not be a perfect fit there > either, but File is a private class, so the inode() function is at least > not part of the public API anymore. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I don't have a strong opinion here. But inode() looks to be called in frame_buffer.cpp, should it be put in frame_buffer.cpp? Best, -Hiro > --- > include/libcamera/base/file.h | 3 +++ > include/libcamera/base/file_descriptor.h | 3 --- > src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++ > src/libcamera/base/file_descriptor.cpp | 25 ------------------------ > src/libcamera/framebuffer.cpp | 5 +++-- > 5 files changed, 29 insertions(+), 30 deletions(-) > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h > index 55e8edd934d4..996751a7ab72 100644 > --- a/include/libcamera/base/file.h > +++ b/include/libcamera/base/file.h > @@ -20,6 +20,8 @@ > > namespace libcamera { > > +class FileDescriptor; > + > class File > { > public: > @@ -66,6 +68,7 @@ public: > bool unmap(uint8_t *addr); > > static bool exists(const std::string &name); > + static ino_t inode(const FileDescriptor &fd); > > private: > LIBCAMERA_DISABLE_COPY(File) > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > index 8d764f8b4a26..5d1594e80801 100644 > --- a/include/libcamera/base/file_descriptor.h > +++ b/include/libcamera/base/file_descriptor.h > @@ -8,7 +8,6 @@ > #pragma once > > #include <memory> > -#include <sys/types.h> > > namespace libcamera { > > @@ -28,8 +27,6 @@ public: > int fd() const { return fd_ ? fd_->fd() : -1; } > FileDescriptor dup() const; > > - ino_t inode() const; > - > private: > class Descriptor > { > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp > index ae4be1f95fa6..7043f9461cf7 100644 > --- a/src/libcamera/base/file.cpp > +++ b/src/libcamera/base/file.cpp > @@ -14,6 +14,7 @@ > #include <sys/types.h> > #include <unistd.h> > > +#include <libcamera/base/file_descriptor.h> > #include <libcamera/base/log.h> > > /** > @@ -472,4 +473,26 @@ bool File::exists(const std::string &name) > return !S_ISDIR(st.st_mode); > } > > +/** > + * \brief Retrieve the inode of a FileDescriptor > + * > + * \return The file descriptor inode on success, or 0 on error > + */ > +ino_t File::inode(const FileDescriptor &fd) > +{ > + if (!fd.isValid()) > + return 0; > + > + struct stat st; > + int ret = fstat(fd.fd(), &st); > + if (ret < 0) { > + ret = -errno; > + LOG(File, Fatal) > + << "Failed to fstat() fd: " << strerror(-ret); > + return 0; > + } > + > + return st.st_ino; > +} > + > } /* namespace libcamera */ > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > index f5f87c56eee8..98d4b4bfd24f 100644 > --- a/src/libcamera/base/file_descriptor.cpp > +++ b/src/libcamera/base/file_descriptor.cpp > @@ -8,7 +8,6 @@ > #include <libcamera/base/file_descriptor.h> > > #include <string.h> > -#include <sys/stat.h> > #include <sys/types.h> > #include <unistd.h> > #include <utility> > @@ -223,30 +222,6 @@ FileDescriptor FileDescriptor::dup() const > return FileDescriptor(fd()); > } > > -/** > - * \brief Retrieve the file descriptor inode > - * > - * \todo Should this move to the File class ? > - * > - * \return The file descriptor inode on success, or 0 on error > - */ > -ino_t FileDescriptor::inode() const > -{ > - if (!isValid()) > - return 0; > - > - struct stat st; > - int ret = fstat(fd_->fd(), &st); > - if (ret < 0) { > - ret = -errno; > - LOG(FileDescriptor, Fatal) > - << "Failed to fstat() fd: " << strerror(-ret); > - return 0; > - } > - > - return st.st_ino; > -} > - > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > { > if (!duplicate) { > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > index 337ea1155a38..f5bcf107d7aa 100644 > --- a/src/libcamera/framebuffer.cpp > +++ b/src/libcamera/framebuffer.cpp > @@ -8,6 +8,7 @@ > #include <libcamera/framebuffer.h> > #include "libcamera/internal/framebuffer.h" > > +#include <libcamera/base/file.h> > #include <libcamera/base/log.h> > > /** > @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > */ > if (plane.fd.fd() != planes_[0].fd.fd()) { > if (!inode) > - inode = planes_[0].fd.inode(); > - if (plane.fd.inode() != inode) { > + inode = File::inode(planes_[0].fd); > + if (File::inode(plane.fd) != inode) { > isContiguous = false; > break; > } > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Mon, Nov 29, 2021 at 01:57:38AM +0200, Laurent Pinchart wrote: > The inode() function has always been a bit of an outcast in the > FileDescriptor class, as it's not related to the core feature provided > by FileDescriptor, a shared ownership wrapper around file descriptors. > Move it to the File class instead. It may not be a perfect fit there > either, but File is a private class, so the inode() function is at least > not part of the public API anymore. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/file.h | 3 +++ > include/libcamera/base/file_descriptor.h | 3 --- > src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++ > src/libcamera/base/file_descriptor.cpp | 25 ------------------------ > src/libcamera/framebuffer.cpp | 5 +++-- > 5 files changed, 29 insertions(+), 30 deletions(-) > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h > index 55e8edd934d4..996751a7ab72 100644 > --- a/include/libcamera/base/file.h > +++ b/include/libcamera/base/file.h > @@ -20,6 +20,8 @@ > > namespace libcamera { > > +class FileDescriptor; > + > class File > { > public: > @@ -66,6 +68,7 @@ public: > bool unmap(uint8_t *addr); > > static bool exists(const std::string &name); > + static ino_t inode(const FileDescriptor &fd); > > private: > LIBCAMERA_DISABLE_COPY(File) > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > index 8d764f8b4a26..5d1594e80801 100644 > --- a/include/libcamera/base/file_descriptor.h > +++ b/include/libcamera/base/file_descriptor.h > @@ -8,7 +8,6 @@ > #pragma once > > #include <memory> > -#include <sys/types.h> > > namespace libcamera { > > @@ -28,8 +27,6 @@ public: > int fd() const { return fd_ ? fd_->fd() : -1; } > FileDescriptor dup() const; > > - ino_t inode() const; > - > private: > class Descriptor > { > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp > index ae4be1f95fa6..7043f9461cf7 100644 > --- a/src/libcamera/base/file.cpp > +++ b/src/libcamera/base/file.cpp > @@ -14,6 +14,7 @@ > #include <sys/types.h> > #include <unistd.h> > > +#include <libcamera/base/file_descriptor.h> > #include <libcamera/base/log.h> > > /** > @@ -472,4 +473,26 @@ bool File::exists(const std::string &name) > return !S_ISDIR(st.st_mode); > } > > +/** > + * \brief Retrieve the inode of a FileDescriptor > + * > + * \return The file descriptor inode on success, or 0 on error > + */ > +ino_t File::inode(const FileDescriptor &fd) > +{ > + if (!fd.isValid()) > + return 0; > + > + struct stat st; > + int ret = fstat(fd.fd(), &st); > + if (ret < 0) { > + ret = -errno; > + LOG(File, Fatal) > + << "Failed to fstat() fd: " << strerror(-ret); > + return 0; > + } > + > + return st.st_ino; > +} > + > } /* namespace libcamera */ > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > index f5f87c56eee8..98d4b4bfd24f 100644 > --- a/src/libcamera/base/file_descriptor.cpp > +++ b/src/libcamera/base/file_descriptor.cpp > @@ -8,7 +8,6 @@ > #include <libcamera/base/file_descriptor.h> > > #include <string.h> > -#include <sys/stat.h> > #include <sys/types.h> > #include <unistd.h> > #include <utility> > @@ -223,30 +222,6 @@ FileDescriptor FileDescriptor::dup() const > return FileDescriptor(fd()); > } > > -/** > - * \brief Retrieve the file descriptor inode > - * > - * \todo Should this move to the File class ? > - * > - * \return The file descriptor inode on success, or 0 on error > - */ > -ino_t FileDescriptor::inode() const > -{ > - if (!isValid()) > - return 0; > - > - struct stat st; > - int ret = fstat(fd_->fd(), &st); > - if (ret < 0) { > - ret = -errno; > - LOG(FileDescriptor, Fatal) > - << "Failed to fstat() fd: " << strerror(-ret); > - return 0; > - } > - > - return st.st_ino; > -} > - > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > { > if (!duplicate) { > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > index 337ea1155a38..f5bcf107d7aa 100644 > --- a/src/libcamera/framebuffer.cpp > +++ b/src/libcamera/framebuffer.cpp > @@ -8,6 +8,7 @@ > #include <libcamera/framebuffer.h> > #include "libcamera/internal/framebuffer.h" > > +#include <libcamera/base/file.h> > #include <libcamera/base/log.h> > > /** > @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > */ > if (plane.fd.fd() != planes_[0].fd.fd()) { > if (!inode) > - inode = planes_[0].fd.inode(); > - if (plane.fd.inode() != inode) { > + inode = File::inode(planes_[0].fd); > + if (File::inode(plane.fd) != inode) { As that's only user I'm not sure I get why a static function is better :/ > isContiguous = false; > break; > } > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Nov 29, 2021 at 03:32:13PM +0100, Jacopo Mondi wrote: > On Mon, Nov 29, 2021 at 01:57:38AM +0200, Laurent Pinchart wrote: > > The inode() function has always been a bit of an outcast in the > > FileDescriptor class, as it's not related to the core feature provided > > by FileDescriptor, a shared ownership wrapper around file descriptors. > > Move it to the File class instead. It may not be a perfect fit there > > either, but File is a private class, so the inode() function is at least > > not part of the public API anymore. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/file.h | 3 +++ > > include/libcamera/base/file_descriptor.h | 3 --- > > src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++ > > src/libcamera/base/file_descriptor.cpp | 25 ------------------------ > > src/libcamera/framebuffer.cpp | 5 +++-- > > 5 files changed, 29 insertions(+), 30 deletions(-) > > > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h > > index 55e8edd934d4..996751a7ab72 100644 > > --- a/include/libcamera/base/file.h > > +++ b/include/libcamera/base/file.h > > @@ -20,6 +20,8 @@ > > > > namespace libcamera { > > > > +class FileDescriptor; > > + > > class File > > { > > public: > > @@ -66,6 +68,7 @@ public: > > bool unmap(uint8_t *addr); > > > > static bool exists(const std::string &name); > > + static ino_t inode(const FileDescriptor &fd); > > > > private: > > LIBCAMERA_DISABLE_COPY(File) > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > > index 8d764f8b4a26..5d1594e80801 100644 > > --- a/include/libcamera/base/file_descriptor.h > > +++ b/include/libcamera/base/file_descriptor.h > > @@ -8,7 +8,6 @@ > > #pragma once > > > > #include <memory> > > -#include <sys/types.h> > > > > namespace libcamera { > > > > @@ -28,8 +27,6 @@ public: > > int fd() const { return fd_ ? fd_->fd() : -1; } > > FileDescriptor dup() const; > > > > - ino_t inode() const; > > - > > private: > > class Descriptor > > { > > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp > > index ae4be1f95fa6..7043f9461cf7 100644 > > --- a/src/libcamera/base/file.cpp > > +++ b/src/libcamera/base/file.cpp > > @@ -14,6 +14,7 @@ > > #include <sys/types.h> > > #include <unistd.h> > > > > +#include <libcamera/base/file_descriptor.h> > > #include <libcamera/base/log.h> > > > > /** > > @@ -472,4 +473,26 @@ bool File::exists(const std::string &name) > > return !S_ISDIR(st.st_mode); > > } > > > > +/** > > + * \brief Retrieve the inode of a FileDescriptor > > + * > > + * \return The file descriptor inode on success, or 0 on error > > + */ > > +ino_t File::inode(const FileDescriptor &fd) > > +{ > > + if (!fd.isValid()) > > + return 0; > > + > > + struct stat st; > > + int ret = fstat(fd.fd(), &st); > > + if (ret < 0) { > > + ret = -errno; > > + LOG(File, Fatal) > > + << "Failed to fstat() fd: " << strerror(-ret); > > + return 0; > > + } > > + > > + return st.st_ino; > > +} > > + > > } /* namespace libcamera */ > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > > index f5f87c56eee8..98d4b4bfd24f 100644 > > --- a/src/libcamera/base/file_descriptor.cpp > > +++ b/src/libcamera/base/file_descriptor.cpp > > @@ -8,7 +8,6 @@ > > #include <libcamera/base/file_descriptor.h> > > > > #include <string.h> > > -#include <sys/stat.h> > > #include <sys/types.h> > > #include <unistd.h> > > #include <utility> > > @@ -223,30 +222,6 @@ FileDescriptor FileDescriptor::dup() const > > return FileDescriptor(fd()); > > } > > > > -/** > > - * \brief Retrieve the file descriptor inode > > - * > > - * \todo Should this move to the File class ? > > - * > > - * \return The file descriptor inode on success, or 0 on error > > - */ > > -ino_t FileDescriptor::inode() const > > -{ > > - if (!isValid()) > > - return 0; > > - > > - struct stat st; > > - int ret = fstat(fd_->fd(), &st); > > - if (ret < 0) { > > - ret = -errno; > > - LOG(FileDescriptor, Fatal) > > - << "Failed to fstat() fd: " << strerror(-ret); > > - return 0; > > - } > > - > > - return st.st_ino; > > -} > > - > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > > { > > if (!duplicate) { > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > > index 337ea1155a38..f5bcf107d7aa 100644 > > --- a/src/libcamera/framebuffer.cpp > > +++ b/src/libcamera/framebuffer.cpp > > @@ -8,6 +8,7 @@ > > #include <libcamera/framebuffer.h> > > #include "libcamera/internal/framebuffer.h" > > > > +#include <libcamera/base/file.h> > > #include <libcamera/base/log.h> > > > > /** > > @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > */ > > if (plane.fd.fd() != planes_[0].fd.fd()) { > > if (!inode) > > - inode = planes_[0].fd.inode(); > > - if (plane.fd.inode() != inode) { > > + inode = File::inode(planes_[0].fd); > > + if (File::inode(plane.fd) != inode) { > > As that's only user I'm not sure I get why a static function is better :/ I've considered that, and then thought the function may be useful somewhere else in the future. As both Hiro and you have the same comment, I'll move it as a static function in this file. > > isContiguous = false; > > break; > > }
Hi Laurent On Mon, Nov 29, 2021 at 06:34:41PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Nov 29, 2021 at 03:32:13PM +0100, Jacopo Mondi wrote: > > On Mon, Nov 29, 2021 at 01:57:38AM +0200, Laurent Pinchart wrote: > > > The inode() function has always been a bit of an outcast in the > > > FileDescriptor class, as it's not related to the core feature provided > > > by FileDescriptor, a shared ownership wrapper around file descriptors. > > > Move it to the File class instead. It may not be a perfect fit there > > > either, but File is a private class, so the inode() function is at least > > > not part of the public API anymore. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/base/file.h | 3 +++ > > > include/libcamera/base/file_descriptor.h | 3 --- > > > src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++ > > > src/libcamera/base/file_descriptor.cpp | 25 ------------------------ > > > src/libcamera/framebuffer.cpp | 5 +++-- > > > 5 files changed, 29 insertions(+), 30 deletions(-) > > > > > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h > > > index 55e8edd934d4..996751a7ab72 100644 > > > --- a/include/libcamera/base/file.h > > > +++ b/include/libcamera/base/file.h > > > @@ -20,6 +20,8 @@ > > > > > > namespace libcamera { > > > > > > +class FileDescriptor; > > > + > > > class File > > > { > > > public: > > > @@ -66,6 +68,7 @@ public: > > > bool unmap(uint8_t *addr); > > > > > > static bool exists(const std::string &name); > > > + static ino_t inode(const FileDescriptor &fd); > > > > > > private: > > > LIBCAMERA_DISABLE_COPY(File) > > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h > > > index 8d764f8b4a26..5d1594e80801 100644 > > > --- a/include/libcamera/base/file_descriptor.h > > > +++ b/include/libcamera/base/file_descriptor.h > > > @@ -8,7 +8,6 @@ > > > #pragma once > > > > > > #include <memory> > > > -#include <sys/types.h> > > > > > > namespace libcamera { > > > > > > @@ -28,8 +27,6 @@ public: > > > int fd() const { return fd_ ? fd_->fd() : -1; } > > > FileDescriptor dup() const; > > > > > > - ino_t inode() const; > > > - > > > private: > > > class Descriptor > > > { > > > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp > > > index ae4be1f95fa6..7043f9461cf7 100644 > > > --- a/src/libcamera/base/file.cpp > > > +++ b/src/libcamera/base/file.cpp > > > @@ -14,6 +14,7 @@ > > > #include <sys/types.h> > > > #include <unistd.h> > > > > > > +#include <libcamera/base/file_descriptor.h> > > > #include <libcamera/base/log.h> > > > > > > /** > > > @@ -472,4 +473,26 @@ bool File::exists(const std::string &name) > > > return !S_ISDIR(st.st_mode); > > > } > > > > > > +/** > > > + * \brief Retrieve the inode of a FileDescriptor > > > + * > > > + * \return The file descriptor inode on success, or 0 on error > > > + */ > > > +ino_t File::inode(const FileDescriptor &fd) > > > +{ > > > + if (!fd.isValid()) > > > + return 0; > > > + > > > + struct stat st; > > > + int ret = fstat(fd.fd(), &st); > > > + if (ret < 0) { > > > + ret = -errno; > > > + LOG(File, Fatal) > > > + << "Failed to fstat() fd: " << strerror(-ret); > > > + return 0; > > > + } > > > + > > > + return st.st_ino; > > > +} > > > + > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp > > > index f5f87c56eee8..98d4b4bfd24f 100644 > > > --- a/src/libcamera/base/file_descriptor.cpp > > > +++ b/src/libcamera/base/file_descriptor.cpp > > > @@ -8,7 +8,6 @@ > > > #include <libcamera/base/file_descriptor.h> > > > > > > #include <string.h> > > > -#include <sys/stat.h> > > > #include <sys/types.h> > > > #include <unistd.h> > > > #include <utility> > > > @@ -223,30 +222,6 @@ FileDescriptor FileDescriptor::dup() const > > > return FileDescriptor(fd()); > > > } > > > > > > -/** > > > - * \brief Retrieve the file descriptor inode > > > - * > > > - * \todo Should this move to the File class ? > > > - * > > > - * \return The file descriptor inode on success, or 0 on error > > > - */ > > > -ino_t FileDescriptor::inode() const > > > -{ > > > - if (!isValid()) > > > - return 0; > > > - > > > - struct stat st; > > > - int ret = fstat(fd_->fd(), &st); > > > - if (ret < 0) { > > > - ret = -errno; > > > - LOG(FileDescriptor, Fatal) > > > - << "Failed to fstat() fd: " << strerror(-ret); > > > - return 0; > > > - } > > > - > > > - return st.st_ino; > > > -} > > > - > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > > > { > > > if (!duplicate) { > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp > > > index 337ea1155a38..f5bcf107d7aa 100644 > > > --- a/src/libcamera/framebuffer.cpp > > > +++ b/src/libcamera/framebuffer.cpp > > > @@ -8,6 +8,7 @@ > > > #include <libcamera/framebuffer.h> > > > #include "libcamera/internal/framebuffer.h" > > > > > > +#include <libcamera/base/file.h> > > > #include <libcamera/base/log.h> > > > > > > /** > > > @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > > */ > > > if (plane.fd.fd() != planes_[0].fd.fd()) { > > > if (!inode) > > > - inode = planes_[0].fd.inode(); > > > - if (plane.fd.inode() != inode) { > > > + inode = File::inode(planes_[0].fd); > > > + if (File::inode(plane.fd) != inode) { > > > > As that's only user I'm not sure I get why a static function is better :/ > > I've considered that, and then thought the function may be useful > somewhere else in the future. As both Hiro and you have the same > comment, I'll move it as a static function in this file. > I was suggesting to keep it in FileDescriptor, but since we have two file descriptors now I understand it's not a good idea. Move it wherever is better ! > > > isContiguous = false; > > > break; > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h index 55e8edd934d4..996751a7ab72 100644 --- a/include/libcamera/base/file.h +++ b/include/libcamera/base/file.h @@ -20,6 +20,8 @@ namespace libcamera { +class FileDescriptor; + class File { public: @@ -66,6 +68,7 @@ public: bool unmap(uint8_t *addr); static bool exists(const std::string &name); + static ino_t inode(const FileDescriptor &fd); private: LIBCAMERA_DISABLE_COPY(File) diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h index 8d764f8b4a26..5d1594e80801 100644 --- a/include/libcamera/base/file_descriptor.h +++ b/include/libcamera/base/file_descriptor.h @@ -8,7 +8,6 @@ #pragma once #include <memory> -#include <sys/types.h> namespace libcamera { @@ -28,8 +27,6 @@ public: int fd() const { return fd_ ? fd_->fd() : -1; } FileDescriptor dup() const; - ino_t inode() const; - private: class Descriptor { diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp index ae4be1f95fa6..7043f9461cf7 100644 --- a/src/libcamera/base/file.cpp +++ b/src/libcamera/base/file.cpp @@ -14,6 +14,7 @@ #include <sys/types.h> #include <unistd.h> +#include <libcamera/base/file_descriptor.h> #include <libcamera/base/log.h> /** @@ -472,4 +473,26 @@ bool File::exists(const std::string &name) return !S_ISDIR(st.st_mode); } +/** + * \brief Retrieve the inode of a FileDescriptor + * + * \return The file descriptor inode on success, or 0 on error + */ +ino_t File::inode(const FileDescriptor &fd) +{ + if (!fd.isValid()) + return 0; + + struct stat st; + int ret = fstat(fd.fd(), &st); + if (ret < 0) { + ret = -errno; + LOG(File, Fatal) + << "Failed to fstat() fd: " << strerror(-ret); + return 0; + } + + return st.st_ino; +} + } /* namespace libcamera */ diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp index f5f87c56eee8..98d4b4bfd24f 100644 --- a/src/libcamera/base/file_descriptor.cpp +++ b/src/libcamera/base/file_descriptor.cpp @@ -8,7 +8,6 @@ #include <libcamera/base/file_descriptor.h> #include <string.h> -#include <sys/stat.h> #include <sys/types.h> #include <unistd.h> #include <utility> @@ -223,30 +222,6 @@ FileDescriptor FileDescriptor::dup() const return FileDescriptor(fd()); } -/** - * \brief Retrieve the file descriptor inode - * - * \todo Should this move to the File class ? - * - * \return The file descriptor inode on success, or 0 on error - */ -ino_t FileDescriptor::inode() const -{ - if (!isValid()) - return 0; - - struct stat st; - int ret = fstat(fd_->fd(), &st); - if (ret < 0) { - ret = -errno; - LOG(FileDescriptor, Fatal) - << "Failed to fstat() fd: " << strerror(-ret); - return 0; - } - - return st.st_ino; -} - FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) { if (!duplicate) { diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp index 337ea1155a38..f5bcf107d7aa 100644 --- a/src/libcamera/framebuffer.cpp +++ b/src/libcamera/framebuffer.cpp @@ -8,6 +8,7 @@ #include <libcamera/framebuffer.h> #include "libcamera/internal/framebuffer.h" +#include <libcamera/base/file.h> #include <libcamera/base/log.h> /** @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) */ if (plane.fd.fd() != planes_[0].fd.fd()) { if (!inode) - inode = planes_[0].fd.inode(); - if (plane.fd.inode() != inode) { + inode = File::inode(planes_[0].fd); + if (File::inode(plane.fd) != inode) { isContiguous = false; break; }
The inode() function has always been a bit of an outcast in the FileDescriptor class, as it's not related to the core feature provided by FileDescriptor, a shared ownership wrapper around file descriptors. Move it to the File class instead. It may not be a perfect fit there either, but File is a private class, so the inode() function is at least not part of the public API anymore. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/base/file.h | 3 +++ include/libcamera/base/file_descriptor.h | 3 --- src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++ src/libcamera/base/file_descriptor.cpp | 25 ------------------------ src/libcamera/framebuffer.cpp | 5 +++-- 5 files changed, 29 insertions(+), 30 deletions(-)