| Message ID | 20211130033820.18235-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thank you for the patch. On Tue, Nov 30, 2021 at 12:38 PM 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. > As it's only used in the FrameBuffer implementation, move it to > frame_buffer.cpp as a static function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > --- > Changes since v3: > > - Move the inode function to frame_buffer.cpp > --- > include/libcamera/base/file.h | 2 ++ > include/libcamera/base/file_descriptor.h | 3 --- > src/libcamera/base/file.cpp | 1 + > src/libcamera/base/file_descriptor.cpp | 25 --------------------- > src/libcamera/framebuffer.cpp | 28 ++++++++++++++++++++++-- > 5 files changed, 29 insertions(+), 30 deletions(-) > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h > index 55e8edd934d4..9b62871b8230 100644 > --- a/include/libcamera/base/file.h > +++ b/include/libcamera/base/file.h > @@ -20,6 +20,8 @@ > > namespace libcamera { > > +class FileDescriptor; > + > class File > { > public: > 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..a04dfacdc102 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> > > /** > 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..b1635f49b68a 100644 > --- a/src/libcamera/framebuffer.cpp > +++ b/src/libcamera/framebuffer.cpp > @@ -8,6 +8,9 @@ > #include <libcamera/framebuffer.h> > #include "libcamera/internal/framebuffer.h" > > +#include <sys/stat.h> > + > +#include <libcamera/base/file.h> > #include <libcamera/base/log.h> > > /** > @@ -207,6 +210,27 @@ FrameBuffer::Private::Private() > * \brief The plane length in bytes > */ > > +namespace { > + > +ino_t fileDescriptorInode(const FileDescriptor &fd) > +{ > + if (!fd.isValid()) > + return 0; > + > + struct stat st; > + int ret = fstat(fd.fd(), &st); > + if (ret < 0) { > + ret = -errno; > + LOG(Buffer, Fatal) > + << "Failed to fstat() fd: " << strerror(-ret); > + return 0; > + } > + > + return st.st_ino; > +} > + > +} /* namespace */ > + > /** > * \brief Construct a FrameBuffer with an array of planes > * \param[in] planes The frame memory planes > @@ -236,8 +260,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 = fileDescriptorInode(planes_[0].fd); > + if (fileDescriptorInode(plane.fd) != inode) { > isContiguous = false; > break; > } > -- > Regards, > > Laurent Pinchart >
Hi Laurent On Tue, Nov 30, 2021 at 05:38:01AM +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. > As it's only used in the FrameBuffer implementation, move it to > frame_buffer.cpp as a static function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v3: > > - Move the inode function to frame_buffer.cpp > --- > include/libcamera/base/file.h | 2 ++ > include/libcamera/base/file_descriptor.h | 3 --- > src/libcamera/base/file.cpp | 1 + > src/libcamera/base/file_descriptor.cpp | 25 --------------------- > src/libcamera/framebuffer.cpp | 28 ++++++++++++++++++++++-- > 5 files changed, 29 insertions(+), 30 deletions(-) > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h > index 55e8edd934d4..9b62871b8230 100644 > --- a/include/libcamera/base/file.h > +++ b/include/libcamera/base/file.h > @@ -20,6 +20,8 @@ > > namespace libcamera { > > +class FileDescriptor; > + Is this needed as nothing is added to file.h ? > class File > { > public: > 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..a04dfacdc102 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> Same question > #include <libcamera/base/log.h> > > /** > 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..b1635f49b68a 100644 > --- a/src/libcamera/framebuffer.cpp > +++ b/src/libcamera/framebuffer.cpp > @@ -8,6 +8,9 @@ > #include <libcamera/framebuffer.h> > #include "libcamera/internal/framebuffer.h" > > +#include <sys/stat.h> > + > +#include <libcamera/base/file.h> Do you need file.h or rather file_descriptor.h for the new function ? Thanks j > #include <libcamera/base/log.h> > > /** > @@ -207,6 +210,27 @@ FrameBuffer::Private::Private() > * \brief The plane length in bytes > */ > > +namespace { > + > +ino_t fileDescriptorInode(const FileDescriptor &fd) > +{ > + if (!fd.isValid()) > + return 0; > + > + struct stat st; > + int ret = fstat(fd.fd(), &st); > + if (ret < 0) { > + ret = -errno; > + LOG(Buffer, Fatal) > + << "Failed to fstat() fd: " << strerror(-ret); > + return 0; > + } > + > + return st.st_ino; > +} > + > +} /* namespace */ > + > /** > * \brief Construct a FrameBuffer with an array of planes > * \param[in] planes The frame memory planes > @@ -236,8 +260,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 = fileDescriptorInode(planes_[0].fd); > + if (fileDescriptorInode(plane.fd) != inode) { > isContiguous = false; > break; > } > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Tue, Nov 30, 2021 at 08:50:11AM +0100, Jacopo Mondi wrote: > On Tue, Nov 30, 2021 at 05:38:01AM +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. > > As it's only used in the FrameBuffer implementation, move it to > > frame_buffer.cpp as a static function. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v3: > > > > - Move the inode function to frame_buffer.cpp > > --- > > include/libcamera/base/file.h | 2 ++ > > include/libcamera/base/file_descriptor.h | 3 --- > > src/libcamera/base/file.cpp | 1 + > > src/libcamera/base/file_descriptor.cpp | 25 --------------------- > > src/libcamera/framebuffer.cpp | 28 ++++++++++++++++++++++-- > > 5 files changed, 29 insertions(+), 30 deletions(-) > > > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h > > index 55e8edd934d4..9b62871b8230 100644 > > --- a/include/libcamera/base/file.h > > +++ b/include/libcamera/base/file.h > > @@ -20,6 +20,8 @@ > > > > namespace libcamera { > > > > +class FileDescriptor; > > + > > Is this needed as nothing is added to file.h ? Absolutely not, it's an incorrect leftover. > > class File > > { > > public: > > 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..a04dfacdc102 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> > > Same question Same answer :-) > > #include <libcamera/base/log.h> > > > > /** > > 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..b1635f49b68a 100644 > > --- a/src/libcamera/framebuffer.cpp > > +++ b/src/libcamera/framebuffer.cpp > > @@ -8,6 +8,9 @@ > > #include <libcamera/framebuffer.h> > > #include "libcamera/internal/framebuffer.h" > > > > +#include <sys/stat.h> > > + > > +#include <libcamera/base/file.h> > > Do you need file.h or rather file_descriptor.h for the new function ? I really messed up this one, haven't I ? I'll fix it. > > #include <libcamera/base/log.h> > > > > /** > > @@ -207,6 +210,27 @@ FrameBuffer::Private::Private() > > * \brief The plane length in bytes > > */ > > > > +namespace { > > + > > +ino_t fileDescriptorInode(const FileDescriptor &fd) > > +{ > > + if (!fd.isValid()) > > + return 0; > > + > > + struct stat st; > > + int ret = fstat(fd.fd(), &st); > > + if (ret < 0) { > > + ret = -errno; > > + LOG(Buffer, Fatal) > > + << "Failed to fstat() fd: " << strerror(-ret); > > + return 0; > > + } > > + > > + return st.st_ino; > > +} > > + > > +} /* namespace */ > > + > > /** > > * \brief Construct a FrameBuffer with an array of planes > > * \param[in] planes The frame memory planes > > @@ -236,8 +260,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 = fileDescriptorInode(planes_[0].fd); > > + if (fileDescriptorInode(plane.fd) != inode) { > > isContiguous = false; > > break; > > }
diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h index 55e8edd934d4..9b62871b8230 100644 --- a/include/libcamera/base/file.h +++ b/include/libcamera/base/file.h @@ -20,6 +20,8 @@ namespace libcamera { +class FileDescriptor; + class File { public: 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..a04dfacdc102 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> /** 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..b1635f49b68a 100644 --- a/src/libcamera/framebuffer.cpp +++ b/src/libcamera/framebuffer.cpp @@ -8,6 +8,9 @@ #include <libcamera/framebuffer.h> #include "libcamera/internal/framebuffer.h" +#include <sys/stat.h> + +#include <libcamera/base/file.h> #include <libcamera/base/log.h> /** @@ -207,6 +210,27 @@ FrameBuffer::Private::Private() * \brief The plane length in bytes */ +namespace { + +ino_t fileDescriptorInode(const FileDescriptor &fd) +{ + if (!fd.isValid()) + return 0; + + struct stat st; + int ret = fstat(fd.fd(), &st); + if (ret < 0) { + ret = -errno; + LOG(Buffer, Fatal) + << "Failed to fstat() fd: " << strerror(-ret); + return 0; + } + + return st.st_ino; +} + +} /* namespace */ + /** * \brief Construct a FrameBuffer with an array of planes * \param[in] planes The frame memory planes @@ -236,8 +260,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 = fileDescriptorInode(planes_[0].fd); + if (fileDescriptorInode(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. As it's only used in the FrameBuffer implementation, move it to frame_buffer.cpp as a static function. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v3: - Move the inode function to frame_buffer.cpp --- include/libcamera/base/file.h | 2 ++ include/libcamera/base/file_descriptor.h | 3 --- src/libcamera/base/file.cpp | 1 + src/libcamera/base/file_descriptor.cpp | 25 --------------------- src/libcamera/framebuffer.cpp | 28 ++++++++++++++++++++++-- 5 files changed, 29 insertions(+), 30 deletions(-)