| Message ID | 20210902042303.2254-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On 02/09/2021 05:22, Laurent Pinchart wrote: > The inode is useful to check if two file descriptors refer to the same > file. Add a function to retrieve it. Aha, great, and I expect this works on virtual-fs so dma-bufs will be comparable? (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media) > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/file_descriptor.h | 3 +++ > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h > index d514aac7697b..988f9b7a3d25 100644 > --- a/include/libcamera/file_descriptor.h > +++ b/include/libcamera/file_descriptor.h > @@ -8,6 +8,7 @@ > #define __LIBCAMERA_FILE_DESCRIPTOR_H__ > > #include <memory> > +#include <sys/types.h> > > namespace libcamera { > > @@ -27,6 +28,8 @@ public: > int fd() const { return fd_ ? fd_->fd() : -1; } > FileDescriptor dup() const; > > + ino_t inode() const; > + > private: > class Descriptor > { > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp > index 9f9ebc81f738..d55e2b100b6d 100644 > --- a/src/libcamera/file_descriptor.cpp > +++ b/src/libcamera/file_descriptor.cpp > @@ -8,6 +8,8 @@ > #include <libcamera/file_descriptor.h> > > #include <string.h> > +#include <sys/stat.h> > +#include <sys/types.h> > #include <unistd.h> > #include <utility> > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const > return FileDescriptor(fd()); > } > > +/** > + * \brief Retrieve the file descriptor inode > + * > + * \todo Should this move to the File class ? I don't think so. File could implement an operator== which checks that the inode of both is the same? Two File()'s with the same inode are the same, but two FileDescriptors have ... separate file descriptors, so while they point to the same location, they are not the same... I guess a FileDescriptor is just a pointer, and the inode is the pointer value ;-) > + * > + * \return The file descriptor inode on success, or 0 on error > + */ > +ino_t FileDescriptor::inode() const > +{ > + if (!fd_ || fd_->fd() == -1) > + return 0; > + > + struct stat st; > + int ret = fstat(fd_->fd(), &st); > + if (ret < 0) > + return 0; > + > + return st.st_ino; I'm torn ... we could parse this when we first construct, and cache the inode. That would save the system call on later comparisons, which may occur more than once on some FileDescriptors, but ... never on others. Or perhaps, as the inode can't be changed underneath the fd, it could simply be cached on the first call ... Do you see any value in putting an operator== in here? It seems reasonable to say that if two FileDescriptors have the same inode, they are == ... however - they may of course be dup()s, with different FDs so we might still want to distinguish that.. Anyway, a lot of that is dependant upon how it ends up getting used... I think it's a good thing to add. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > +} > + > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > { > if (!duplicate) { >
Hi Kieran, On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote: > On 02/09/2021 05:22, Laurent Pinchart wrote: > > The inode is useful to check if two file descriptors refer to the same > > file. Add a function to retrieve it. > > Aha, great, and I expect this works on virtual-fs so dma-bufs will be > comparable? That's the goal, yes. Dmabuf instances are associated with an inode, so two different file descriptors that have the same inode are guaranteed to refer to the same dmabuf (provided the file descriptors reference a dmabuf, inodes are not unique globally). This works with dup() or when sending the file descriptor over an IPC mechanism. > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media) For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer will create multiple distinct dmabuf instances, and there's no way for userspace to know that they related to the same underlying buffer. Buffers allocated by DRM doesn't suffer from this issue, the always return the same dmabuf when exporting the buffer multiple times. This should be fixable on the V4L2 side. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/file_descriptor.h | 3 +++ > > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h > > index d514aac7697b..988f9b7a3d25 100644 > > --- a/include/libcamera/file_descriptor.h > > +++ b/include/libcamera/file_descriptor.h > > @@ -8,6 +8,7 @@ > > #define __LIBCAMERA_FILE_DESCRIPTOR_H__ > > > > #include <memory> > > +#include <sys/types.h> > > > > namespace libcamera { > > > > @@ -27,6 +28,8 @@ public: > > int fd() const { return fd_ ? fd_->fd() : -1; } > > FileDescriptor dup() const; > > > > + ino_t inode() const; > > + > > private: > > class Descriptor > > { > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp > > index 9f9ebc81f738..d55e2b100b6d 100644 > > --- a/src/libcamera/file_descriptor.cpp > > +++ b/src/libcamera/file_descriptor.cpp > > @@ -8,6 +8,8 @@ > > #include <libcamera/file_descriptor.h> > > > > #include <string.h> > > +#include <sys/stat.h> > > +#include <sys/types.h> > > #include <unistd.h> > > #include <utility> > > > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const > > return FileDescriptor(fd()); > > } > > > > +/** > > + * \brief Retrieve the file descriptor inode > > + * > > + * \todo Should this move to the File class ? > > I don't think so. > > File could implement an operator== which checks that the inode of both > is the same? > > Two File()'s with the same inode are the same, but two FileDescriptors > have ... separate file descriptors, so while they point to the same > location, they are not the same... It depends how you define "file" :-) Calling open() twice on the same path (thus referring to the same inode) will create two files on the kernel side, each of them maintaining their own context (such as the read/write pointer and other flags). We could compare File instances based on different underlying objects, including - The path (with or without following symlinks) - The kernel file instance (not sure how though, kcmp() may be an option) - The inode Note that inode comparison isn't enough, as they're not globally unique. They are usually unique within a file system (so we can compare the stat st_dev and st_ino fields), but that's not always guaranteed (on btrfs for instance, see https://lwn.net/Articles/866582/). > I guess a FileDescriptor is just a pointer, and the inode is the pointer > value ;-) There are three levels, inode (within the context of a filesystem), file (from a kernel point of view) and file descriptor. You can have multiple files referring to the same inode (for instance by open()ing the same path twice), and multiple file descriptors referring to the same file (for instance by calling dup() or sending the file descriptor over IPC). So FileDescriptor is indeed a pointer, but it's the top of the stack, while the inode is at the bottom. > > + * > > + * \return The file descriptor inode on success, or 0 on error > > + */ > > +ino_t FileDescriptor::inode() const > > +{ > > + if (!fd_ || fd_->fd() == -1) > > + return 0; > > + > > + struct stat st; > > + int ret = fstat(fd_->fd(), &st); > > + if (ret < 0) > > + return 0; > > + > > + return st.st_ino; > > I'm torn ... we could parse this when we first construct, and cache the > inode. I didn't to avoid the overhead that is not required in the majority of use cases. > That would save the system call on later comparisons, which may occur > more than once on some FileDescriptors, but ... never on others. > > > Or perhaps, as the inode can't be changed underneath the fd, it could > simply be cached on the first call ... I've thought about that too, but wasn't very keen on adding a field to the FileDescriptor class for internal purpose only, as inode() isn't meant to be a public API. If FileDescriptor was moved to base/ that may be different, then we could consider which features belong to File and which belong to FileDescriptor, and create a nice API. For now, I've decided to just add FileDescriptor::inode() without caching, and add a \todo. One option, if we decide to keep this in FileDescriptor, is to cache it in FileDescriptor::Descriptor. > Do you see any value in putting an operator== in here? It seems > reasonable to say that if two FileDescriptors have the same inode, they > are == ... however - they may of course be dup()s, with different FDs so > we might still want to distinguish that.. I think an operator==() for both File and FileDescriptor would make sense, but again we need to define what equality means first :-) I'd rather not do so as part of this series, which I want to merge fast to fixes breakages in the master branch. That's why there's a todo at the top of the patch, I think we can review the design of File and FileDescriptor later. > Anyway, a lot of that is dependant upon how it ends up getting used... > > I think it's a good thing to add. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > +} > > + > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > > { > > if (!duplicate) {
Hi Laurent, thank you for the patch. On Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kieran, > > On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote: > > On 02/09/2021 05:22, Laurent Pinchart wrote: > > > The inode is useful to check if two file descriptors refer to the same > > > file. Add a function to retrieve it. > > > > Aha, great, and I expect this works on virtual-fs so dma-bufs will be > > comparable? > > That's the goal, yes. > > Dmabuf instances are associated with an inode, so two different file > descriptors that have the same inode are guaranteed to refer to the same > dmabuf (provided the file descriptors reference a dmabuf, inodes are not > unique globally). This works with dup() or when sending the file > descriptor over an IPC mechanism. > > > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media) > > For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer > will create multiple distinct dmabuf instances, and there's no way for > userspace to know that they related to the same underlying buffer. > Buffers allocated by DRM doesn't suffer from this issue, the always > return the same dmabuf when exporting the buffer multiple times. This > should be fixable on the V4L2 side. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/file_descriptor.h | 3 +++ > > > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++ > > > 2 files changed, 25 insertions(+) > > > > > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h > > > index d514aac7697b..988f9b7a3d25 100644 > > > --- a/include/libcamera/file_descriptor.h > > > +++ b/include/libcamera/file_descriptor.h > > > @@ -8,6 +8,7 @@ > > > #define __LIBCAMERA_FILE_DESCRIPTOR_H__ > > > > > > #include <memory> > > > +#include <sys/types.h> > > > > > > namespace libcamera { > > > > > > @@ -27,6 +28,8 @@ public: > > > int fd() const { return fd_ ? fd_->fd() : -1; } > > > FileDescriptor dup() const; > > > > > > + ino_t inode() const; > > > + > > > private: > > > class Descriptor > > > { > > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp > > > index 9f9ebc81f738..d55e2b100b6d 100644 > > > --- a/src/libcamera/file_descriptor.cpp > > > +++ b/src/libcamera/file_descriptor.cpp > > > @@ -8,6 +8,8 @@ > > > #include <libcamera/file_descriptor.h> > > > > > > #include <string.h> > > > +#include <sys/stat.h> > > > +#include <sys/types.h> > > > #include <unistd.h> > > > #include <utility> > > > > > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const > > > return FileDescriptor(fd()); > > > } > > > > > > +/** > > > + * \brief Retrieve the file descriptor inode > > > + * > > > + * \todo Should this move to the File class ? > > > > I don't think so. > > > > File could implement an operator== which checks that the inode of both > > is the same? > > > > Two File()'s with the same inode are the same, but two FileDescriptors > > have ... separate file descriptors, so while they point to the same > > location, they are not the same... > > It depends how you define "file" :-) Calling open() twice on the same > path (thus referring to the same inode) will create two files on the > kernel side, each of them maintaining their own context (such as the > read/write pointer and other flags). > > We could compare File instances based on different underlying objects, > including > > - The path (with or without following symlinks) > - The kernel file instance (not sure how though, kcmp() may be an option) > - The inode > > Note that inode comparison isn't enough, as they're not globally unique. > They are usually unique within a file system (so we can compare the stat > st_dev and st_ino fields), but that's not always guaranteed (on btrfs > for instance, see https://lwn.net/Articles/866582/). > > > I guess a FileDescriptor is just a pointer, and the inode is the pointer > > value ;-) > > There are three levels, inode (within the context of a filesystem), file > (from a kernel point of view) and file descriptor. You can have multiple > files referring to the same inode (for instance by open()ing the same > path twice), and multiple file descriptors referring to the same file > (for instance by calling dup() or sending the file descriptor over IPC). > > So FileDescriptor is indeed a pointer, but it's the top of the stack, > while the inode is at the bottom. > > > > + * > > > + * \return The file descriptor inode on success, or 0 on error > > > + */ > > > +ino_t FileDescriptor::inode() const > > > +{ > > > + if (!fd_ || fd_->fd() == -1) > > > + return 0; if (!isValid()) return 0; As far as I look the FileDescriptor code, if isValid() is true, fd_ must be valid. > > > + > > > + struct stat st; > > > + int ret = fstat(fd_->fd(), &st); > > > + if (ret < 0) > > > + return 0; Shall we add the log? LOG(FileDescriptor, Error) << "fstat failed: " << errno; > > > + > > > + return st.st_ino; I am a bit concerned two invalid file descriptors are regarded thanks to returning 0. Perhaps, let the return type be int64_t and cast ino_t to int64_t? Or since ino_t is ulong_t=unsinged long=uint64_t (in 64bit machine), shall we return pair(ino_t, error) or error + pointer argument ino_t? > > > > I'm torn ... we could parse this when we first construct, and cache the > > inode. > > I didn't to avoid the overhead that is not required in the majority of > use cases. > > > That would save the system call on later comparisons, which may occur > > more than once on some FileDescriptors, but ... never on others. > > > > > > Or perhaps, as the inode can't be changed underneath the fd, it could > > simply be cached on the first call ... > > I've thought about that too, but wasn't very keen on adding a field to > the FileDescriptor class for internal purpose only, as inode() isn't > meant to be a public API. If FileDescriptor was moved to base/ that may > be different, then we could consider which features belong to File and > which belong to FileDescriptor, and create a nice API. For now, I've > decided to just add FileDescriptor::inode() without caching, and add a > \todo. > > One option, if we decide to keep this in FileDescriptor, is to cache it > in FileDescriptor::Descriptor. > > > Do you see any value in putting an operator== in here? It seems > > reasonable to say that if two FileDescriptors have the same inode, they > > are == ... however - they may of course be dup()s, with different FDs so > > we might still want to distinguish that.. > > I think an operator==() for both File and FileDescriptor would make > sense, but again we need to define what equality means first :-) I'd > rather not do so as part of this series, which I want to merge fast to > fixes breakages in the master branch. That's why there's a todo at the > top of the patch, I think we can review the design of File and > FileDescriptor later. > I could not imagine the use case of comparing File. For FileDescriptors, it might be useful, I would use just inode comparison manually on a caller side because currently the use cases are small enough (android/camera_device and libcamera/v4l2_device only?). Best Regards, -Hiro > > Anyway, a lot of that is dependant upon how it ends up getting used... > > > > I think it's a good thing to add. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > +} > > > + > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > > > { > > > if (!duplicate) { > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Fri, Sep 03, 2021 at 07:07:47PM +0900, Hirokazu Honda wrote: > On Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart wrote: > > On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote: > > > On 02/09/2021 05:22, Laurent Pinchart wrote: > > > > The inode is useful to check if two file descriptors refer to the same > > > > file. Add a function to retrieve it. > > > > > > Aha, great, and I expect this works on virtual-fs so dma-bufs will be > > > comparable? > > > > That's the goal, yes. > > > > Dmabuf instances are associated with an inode, so two different file > > descriptors that have the same inode are guaranteed to refer to the same > > dmabuf (provided the file descriptors reference a dmabuf, inodes are not > > unique globally). This works with dup() or when sending the file > > descriptor over an IPC mechanism. > > > > > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media) > > > > For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer > > will create multiple distinct dmabuf instances, and there's no way for > > userspace to know that they related to the same underlying buffer. > > Buffers allocated by DRM doesn't suffer from this issue, the always > > return the same dmabuf when exporting the buffer multiple times. This > > should be fixable on the V4L2 side. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > include/libcamera/file_descriptor.h | 3 +++ > > > > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++ > > > > 2 files changed, 25 insertions(+) > > > > > > > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h > > > > index d514aac7697b..988f9b7a3d25 100644 > > > > --- a/include/libcamera/file_descriptor.h > > > > +++ b/include/libcamera/file_descriptor.h > > > > @@ -8,6 +8,7 @@ > > > > #define __LIBCAMERA_FILE_DESCRIPTOR_H__ > > > > > > > > #include <memory> > > > > +#include <sys/types.h> > > > > > > > > namespace libcamera { > > > > > > > > @@ -27,6 +28,8 @@ public: > > > > int fd() const { return fd_ ? fd_->fd() : -1; } > > > > FileDescriptor dup() const; > > > > > > > > + ino_t inode() const; > > > > + > > > > private: > > > > class Descriptor > > > > { > > > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp > > > > index 9f9ebc81f738..d55e2b100b6d 100644 > > > > --- a/src/libcamera/file_descriptor.cpp > > > > +++ b/src/libcamera/file_descriptor.cpp > > > > @@ -8,6 +8,8 @@ > > > > #include <libcamera/file_descriptor.h> > > > > > > > > #include <string.h> > > > > +#include <sys/stat.h> > > > > +#include <sys/types.h> > > > > #include <unistd.h> > > > > #include <utility> > > > > > > > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const > > > > return FileDescriptor(fd()); > > > > } > > > > > > > > +/** > > > > + * \brief Retrieve the file descriptor inode > > > > + * > > > > + * \todo Should this move to the File class ? > > > > > > I don't think so. > > > > > > File could implement an operator== which checks that the inode of both > > > is the same? > > > > > > Two File()'s with the same inode are the same, but two FileDescriptors > > > have ... separate file descriptors, so while they point to the same > > > location, they are not the same... > > > > It depends how you define "file" :-) Calling open() twice on the same > > path (thus referring to the same inode) will create two files on the > > kernel side, each of them maintaining their own context (such as the > > read/write pointer and other flags). > > > > We could compare File instances based on different underlying objects, > > including > > > > - The path (with or without following symlinks) > > - The kernel file instance (not sure how though, kcmp() may be an option) > > - The inode > > > > Note that inode comparison isn't enough, as they're not globally unique. > > They are usually unique within a file system (so we can compare the stat > > st_dev and st_ino fields), but that's not always guaranteed (on btrfs > > for instance, see https://lwn.net/Articles/866582/). > > > > > I guess a FileDescriptor is just a pointer, and the inode is the pointer > > > value ;-) > > > > There are three levels, inode (within the context of a filesystem), file > > (from a kernel point of view) and file descriptor. You can have multiple > > files referring to the same inode (for instance by open()ing the same > > path twice), and multiple file descriptors referring to the same file > > (for instance by calling dup() or sending the file descriptor over IPC). > > > > So FileDescriptor is indeed a pointer, but it's the top of the stack, > > while the inode is at the bottom. > > > > > > + * > > > > + * \return The file descriptor inode on success, or 0 on error > > > > + */ > > > > +ino_t FileDescriptor::inode() const > > > > +{ > > > > + if (!fd_ || fd_->fd() == -1) > > > > + return 0; > > if (!isValid()) > return 0; > > As far as I look the FileDescriptor code, if isValid() is true, fd_ > must be valid. Good point, I'll do that. > > > > + > > > > + struct stat st; > > > > + int ret = fstat(fd_->fd(), &st); > > > > + if (ret < 0) > > > > + return 0; > > Shall we add the log? > LOG(FileDescriptor, Error) << "fstat failed: " << errno; I'll add an error message in v2. > > > > + > > > > + return st.st_ino; > > I am a bit concerned two invalid file descriptors are regarded thanks > to returning 0. > Perhaps, let the return type be int64_t and cast ino_t to int64_t? > Or since ino_t is ulong_t=unsinged long=uint64_t (in 64bit machine), > shall we return pair(ino_t, error) or error + pointer argument ino_t? 0 is never allocated as a valid inode by filesystems, so I think it's safe to use it to indicate an error. There are also a set of special inodes, for instance #define EXT4_BAD_INO 1 /* Bad blocks inode */ #define EXT4_ROOT_INO 2 /* Root inode */ #define EXT4_USR_QUOTA_INO 3 /* User quota inode */ #define EXT4_GRP_QUOTA_INO 4 /* Group quota inode */ #define EXT4_BOOT_LOADER_INO 5 /* Boot loader inode */ #define EXT4_UNDEL_DIR_INO 6 /* Undelete directory inode */ #define EXT4_RESIZE_INO 7 /* Reserved group descriptors inode */ #define EXT4_JOURNAL_INO 8 /* Journal inode */ but that's filesystem-specific. > > > > > > I'm torn ... we could parse this when we first construct, and cache the > > > inode. > > > > I didn't to avoid the overhead that is not required in the majority of > > use cases. > > > > > That would save the system call on later comparisons, which may occur > > > more than once on some FileDescriptors, but ... never on others. > > > > > > > > > Or perhaps, as the inode can't be changed underneath the fd, it could > > > simply be cached on the first call ... > > > > I've thought about that too, but wasn't very keen on adding a field to > > the FileDescriptor class for internal purpose only, as inode() isn't > > meant to be a public API. If FileDescriptor was moved to base/ that may > > be different, then we could consider which features belong to File and > > which belong to FileDescriptor, and create a nice API. For now, I've > > decided to just add FileDescriptor::inode() without caching, and add a > > \todo. > > > > One option, if we decide to keep this in FileDescriptor, is to cache it > > in FileDescriptor::Descriptor. > > > > > Do you see any value in putting an operator== in here? It seems > > > reasonable to say that if two FileDescriptors have the same inode, they > > > are == ... however - they may of course be dup()s, with different FDs so > > > we might still want to distinguish that.. > > > > I think an operator==() for both File and FileDescriptor would make > > sense, but again we need to define what equality means first :-) I'd > > rather not do so as part of this series, which I want to merge fast to > > fixes breakages in the master branch. That's why there's a todo at the > > top of the patch, I think we can review the design of File and > > FileDescriptor later. > > I could not imagine the use case of comparing File. > For FileDescriptors, it might be useful, I would use just inode > comparison manually on a caller side because currently the use cases > are small enough (android/camera_device and libcamera/v4l2_device > only?). We only have one user of the inode() function in this series, so it's indeed hard to know exactly how to design the API. I'd prefer waiting a bit for more use cases to appear before deciding on the semantics of the equality operator. > > > Anyway, a lot of that is dependant upon how it ends up getting used... > > > > > > I think it's a good thing to add. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > +} > > > > + > > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > > > > { > > > > if (!duplicate) {
Hi Laurent, On Fri, Sep 3, 2021 at 7:35 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Fri, Sep 03, 2021 at 07:07:47PM +0900, Hirokazu Honda wrote: > > On Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart wrote: > > > On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote: > > > > On 02/09/2021 05:22, Laurent Pinchart wrote: > > > > > The inode is useful to check if two file descriptors refer to the same > > > > > file. Add a function to retrieve it. > > > > > > > > Aha, great, and I expect this works on virtual-fs so dma-bufs will be > > > > comparable? > > > > > > That's the goal, yes. > > > > > > Dmabuf instances are associated with an inode, so two different file > > > descriptors that have the same inode are guaranteed to refer to the same > > > dmabuf (provided the file descriptors reference a dmabuf, inodes are not > > > unique globally). This works with dup() or when sending the file > > > descriptor over an IPC mechanism. > > > > > > > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media) > > > > > > For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer > > > will create multiple distinct dmabuf instances, and there's no way for > > > userspace to know that they related to the same underlying buffer. > > > Buffers allocated by DRM doesn't suffer from this issue, the always > > > return the same dmabuf when exporting the buffer multiple times. This > > > should be fixable on the V4L2 side. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > include/libcamera/file_descriptor.h | 3 +++ > > > > > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++ > > > > > 2 files changed, 25 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h > > > > > index d514aac7697b..988f9b7a3d25 100644 > > > > > --- a/include/libcamera/file_descriptor.h > > > > > +++ b/include/libcamera/file_descriptor.h > > > > > @@ -8,6 +8,7 @@ > > > > > #define __LIBCAMERA_FILE_DESCRIPTOR_H__ > > > > > > > > > > #include <memory> > > > > > +#include <sys/types.h> > > > > > > > > > > namespace libcamera { > > > > > > > > > > @@ -27,6 +28,8 @@ public: > > > > > int fd() const { return fd_ ? fd_->fd() : -1; } > > > > > FileDescriptor dup() const; > > > > > > > > > > + ino_t inode() const; > > > > > + > > > > > private: > > > > > class Descriptor > > > > > { > > > > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp > > > > > index 9f9ebc81f738..d55e2b100b6d 100644 > > > > > --- a/src/libcamera/file_descriptor.cpp > > > > > +++ b/src/libcamera/file_descriptor.cpp > > > > > @@ -8,6 +8,8 @@ > > > > > #include <libcamera/file_descriptor.h> > > > > > > > > > > #include <string.h> > > > > > +#include <sys/stat.h> > > > > > +#include <sys/types.h> > > > > > #include <unistd.h> > > > > > #include <utility> > > > > > > > > > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const > > > > > return FileDescriptor(fd()); > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Retrieve the file descriptor inode > > > > > + * > > > > > + * \todo Should this move to the File class ? > > > > > > > > I don't think so. > > > > > > > > File could implement an operator== which checks that the inode of both > > > > is the same? > > > > > > > > Two File()'s with the same inode are the same, but two FileDescriptors > > > > have ... separate file descriptors, so while they point to the same > > > > location, they are not the same... > > > > > > It depends how you define "file" :-) Calling open() twice on the same > > > path (thus referring to the same inode) will create two files on the > > > kernel side, each of them maintaining their own context (such as the > > > read/write pointer and other flags). > > > > > > We could compare File instances based on different underlying objects, > > > including > > > > > > - The path (with or without following symlinks) > > > - The kernel file instance (not sure how though, kcmp() may be an option) > > > - The inode > > > > > > Note that inode comparison isn't enough, as they're not globally unique. > > > They are usually unique within a file system (so we can compare the stat > > > st_dev and st_ino fields), but that's not always guaranteed (on btrfs > > > for instance, see https://lwn.net/Articles/866582/). > > > > > > > I guess a FileDescriptor is just a pointer, and the inode is the pointer > > > > value ;-) > > > > > > There are three levels, inode (within the context of a filesystem), file > > > (from a kernel point of view) and file descriptor. You can have multiple > > > files referring to the same inode (for instance by open()ing the same > > > path twice), and multiple file descriptors referring to the same file > > > (for instance by calling dup() or sending the file descriptor over IPC). > > > > > > So FileDescriptor is indeed a pointer, but it's the top of the stack, > > > while the inode is at the bottom. > > > > > > > > + * > > > > > + * \return The file descriptor inode on success, or 0 on error > > > > > + */ > > > > > +ino_t FileDescriptor::inode() const > > > > > +{ > > > > > + if (!fd_ || fd_->fd() == -1) > > > > > + return 0; > > > > if (!isValid()) > > return 0; > > > > As far as I look the FileDescriptor code, if isValid() is true, fd_ > > must be valid. > > Good point, I'll do that. > > > > > > + > > > > > + struct stat st; > > > > > + int ret = fstat(fd_->fd(), &st); > > > > > + if (ret < 0) > > > > > + return 0; > > > > Shall we add the log? > > LOG(FileDescriptor, Error) << "fstat failed: " << errno; > > I'll add an error message in v2. > > > > > > + > > > > > + return st.st_ino; > > > > I am a bit concerned two invalid file descriptors are regarded thanks > > to returning 0. > > Perhaps, let the return type be int64_t and cast ino_t to int64_t? > > Or since ino_t is ulong_t=unsinged long=uint64_t (in 64bit machine), > > shall we return pair(ino_t, error) or error + pointer argument ino_t? > > 0 is never allocated as a valid inode by filesystems, so I think it's > safe to use it to indicate an error. There are also a set of special > inodes, for instance > > #define EXT4_BAD_INO 1 /* Bad blocks inode */ > #define EXT4_ROOT_INO 2 /* Root inode */ > #define EXT4_USR_QUOTA_INO 3 /* User quota inode */ > #define EXT4_GRP_QUOTA_INO 4 /* Group quota inode */ > #define EXT4_BOOT_LOADER_INO 5 /* Boot loader inode */ > #define EXT4_UNDEL_DIR_INO 6 /* Undelete directory inode */ > #define EXT4_RESIZE_INO 7 /* Reserved group descriptors inode */ > #define EXT4_JOURNAL_INO 8 /* Journal inode */ > > but that's filesystem-specific. > I got it. > > > > > > > > I'm torn ... we could parse this when we first construct, and cache the > > > > inode. > > > > > > I didn't to avoid the overhead that is not required in the majority of > > > use cases. > > > > > > > That would save the system call on later comparisons, which may occur > > > > more than once on some FileDescriptors, but ... never on others. > > > > > > > > > > > > Or perhaps, as the inode can't be changed underneath the fd, it could > > > > simply be cached on the first call ... > > > > > > I've thought about that too, but wasn't very keen on adding a field to > > > the FileDescriptor class for internal purpose only, as inode() isn't > > > meant to be a public API. If FileDescriptor was moved to base/ that may > > > be different, then we could consider which features belong to File and > > > which belong to FileDescriptor, and create a nice API. For now, I've > > > decided to just add FileDescriptor::inode() without caching, and add a > > > \todo. > > > > > > One option, if we decide to keep this in FileDescriptor, is to cache it > > > in FileDescriptor::Descriptor. > > > > > > > Do you see any value in putting an operator== in here? It seems > > > > reasonable to say that if two FileDescriptors have the same inode, they > > > > are == ... however - they may of course be dup()s, with different FDs so > > > > we might still want to distinguish that.. > > > > > > I think an operator==() for both File and FileDescriptor would make > > > sense, but again we need to define what equality means first :-) I'd > > > rather not do so as part of this series, which I want to merge fast to > > > fixes breakages in the master branch. That's why there's a todo at the > > > top of the patch, I think we can review the design of File and > > > FileDescriptor later. > > > > I could not imagine the use case of comparing File. > > For FileDescriptors, it might be useful, I would use just inode > > comparison manually on a caller side because currently the use cases > > are small enough (android/camera_device and libcamera/v4l2_device > > only?). > > We only have one user of the inode() function in this series, so it's > indeed hard to know exactly how to design the API. I'd prefer waiting a > bit for more use cases to appear before deciding on the semantics of the > equality operator. > Yeah, I agree. If we want to add, we should start with defining equality and it shall be done later, not this patch series. -Hiro > > > > Anyway, a lot of that is dependant upon how it ends up getting used... > > > > > > > > I think it's a good thing to add. > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > +} > > > > > + > > > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) > > > > > { > > > > > if (!duplicate) { > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h index d514aac7697b..988f9b7a3d25 100644 --- a/include/libcamera/file_descriptor.h +++ b/include/libcamera/file_descriptor.h @@ -8,6 +8,7 @@ #define __LIBCAMERA_FILE_DESCRIPTOR_H__ #include <memory> +#include <sys/types.h> namespace libcamera { @@ -27,6 +28,8 @@ public: int fd() const { return fd_ ? fd_->fd() : -1; } FileDescriptor dup() const; + ino_t inode() const; + private: class Descriptor { diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp index 9f9ebc81f738..d55e2b100b6d 100644 --- a/src/libcamera/file_descriptor.cpp +++ b/src/libcamera/file_descriptor.cpp @@ -8,6 +8,8 @@ #include <libcamera/file_descriptor.h> #include <string.h> +#include <sys/stat.h> +#include <sys/types.h> #include <unistd.h> #include <utility> @@ -221,6 +223,26 @@ 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 (!fd_ || fd_->fd() == -1) + return 0; + + struct stat st; + int ret = fstat(fd_->fd(), &st); + if (ret < 0) + return 0; + + return st.st_ino; +} + FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate) { if (!duplicate) {
The inode is useful to check if two file descriptors refer to the same file. Add a function to retrieve it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/file_descriptor.h | 3 +++ src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+)