[{"id":19272,"web_url":"https://patchwork.libcamera.org/comment/19272/","msgid":"","date":"2021-09-02T08:39:11","subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 02/09/2021 05:22, Laurent Pinchart wrote:\n> The inode is useful to check if two file descriptors refer to the same\n> file. Add a function to retrieve it.\n\nAha, great, and I expect this works on virtual-fs so dma-bufs will be\ncomparable?\n\n(edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media)\n\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/file_descriptor.h | 3 +++\n> src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++\n> 2 files changed, 25 insertions(+)\n> \n> diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h\n> index d514aac7697b..988f9b7a3d25 100644\n> --- a/include/libcamera/file_descriptor.h\n> +++ b/include/libcamera/file_descriptor.h\n> @@ -8,6 +8,7 @@\n> #define __LIBCAMERA_FILE_DESCRIPTOR_H__\n> \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -27,6 +28,8 @@ public:\n> \tint fd() const { return fd_ ? fd_->fd() : -1; }\n> \tFileDescriptor dup() const;\n> \n> +\tino_t inode() const;\n> +\n> private:\n> \tclass Descriptor\n> \t{\n> diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp\n> index 9f9ebc81f738..d55e2b100b6d 100644\n> --- a/src/libcamera/file_descriptor.cpp\n> +++ b/src/libcamera/file_descriptor.cpp\n> @@ -8,6 +8,8 @@\n> #include \n> \n> #include \n> +#include \n> +#include \n> #include \n> #include \n> \n> @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const\n> \treturn FileDescriptor(fd());\n> }\n> \n> +/**\n> + * \\brief Retrieve the file descriptor inode\n> + *\n> + * \\todo Should this move to the File class ?\n\nI don't think so.\n\nFile could implement an operator== which checks that the inode of both\nis the same?\n\nTwo File()'s with the same inode are the same, but two FileDescriptors\nhave ... separate file descriptors, so while they point to the same\nlocation, they are not the same...\n\nI guess a FileDescriptor is just a pointer, and the inode is the pointer\nvalue ;-)\n\n> + *\n> + * \\return The file descriptor inode on success, or 0 on error\n> + */\n> +ino_t FileDescriptor::inode() const\n> +{\n> +\tif (!fd_ || fd_->fd() == -1)\n> +\t\treturn 0;\n> +\n> +\tstruct stat st;\n> +\tint ret = fstat(fd_->fd(), &st);\n> +\tif (ret < 0)\n> +\t\treturn 0;\n> +\n> +\treturn st.st_ino;\n\nI'm torn ... we could parse this when we first construct, and cache the\ninode.\n\nThat would save the system call on later comparisons, which may occur\nmore than once on some FileDescriptors, but ... never on others.\n\n\nOr perhaps, as the inode can't be changed underneath the fd, it could\nsimply be cached on the first call ...\n\n\nDo you see any value in putting an operator== in here? It seems\nreasonable to say that if two FileDescriptors have the same inode, they\nare == ... however - they may of course be dup()s, with different FDs so\nwe might still want to distinguish that..\n\n\nAnyway, a lot of that is dependant upon how it ends up getting used...\n\nI think it's a good thing to add.\n\nReviewed-by: Kieran Bingham \n\n> +}\n> +\n> FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> {\n> \tif (!duplicate) {\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AF49DBDC71\n\tfor ;\n\tThu, 2 Sep 2021 08:39:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24B3C69166;\n\tThu, 2 Sep 2021 10:39:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04BC560254\n\tfor ;\n\tThu, 2 Sep 2021 10:39:14 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80D9A45E;\n\tThu, 2 Sep 2021 10:39:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cJ2/xkyM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630571953;\n\tbh=Wsbl6spFwQJD7AvL/GmQQs/d4iuUOQVTWiIGQs6DGlE=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=cJ2/xkyMiBPDzUKo+hh1RmsK7uTQzI8Gd+KAWc5kL+C5hwtdqcnBAGnztoMGFE4Lp\n\tECRAgAkYe38Z209rU8WFrUgSJocwyFZ/Qy/3nQJBnNFmQkAZTX57tdHLuGgkg86Sw6\n\tfcBHCWDthAIxpmJ4eTWLfTYV9Shong+D0fMWVs9M=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-3-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Thu, 2 Sep 2021 09:39:11 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210902042303.2254-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19301,"web_url":"https://patchwork.libcamera.org/comment/19301/","msgid":"","date":"2021-09-02T17:56:12","subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote:\n> On 02/09/2021 05:22, Laurent Pinchart wrote:\n> > The inode is useful to check if two file descriptors refer to the same\n> > file. Add a function to retrieve it.\n> \n> Aha, great, and I expect this works on virtual-fs so dma-bufs will be\n> comparable?\n\nThat's the goal, yes.\n\nDmabuf instances are associated with an inode, so two different file\ndescriptors that have the same inode are guaranteed to refer to the same\ndmabuf (provided the file descriptors reference a dmabuf, inodes are not\nunique globally). This works with dup() or when sending the file\ndescriptor over an IPC mechanism.\n\n> (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media)\n\nFor the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer\nwill create multiple distinct dmabuf instances, and there's no way for\nuserspace to know that they related to the same underlying buffer.\nBuffers allocated by DRM doesn't suffer from this issue, the always\nreturn the same dmabuf when exporting the buffer multiple times. This\nshould be fixable on the V4L2 side.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/file_descriptor.h | 3 +++\n> > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++\n> > 2 files changed, 25 insertions(+)\n> > \n> > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h\n> > index d514aac7697b..988f9b7a3d25 100644\n> > --- a/include/libcamera/file_descriptor.h\n> > +++ b/include/libcamera/file_descriptor.h\n> > @@ -8,6 +8,7 @@\n> > #define __LIBCAMERA_FILE_DESCRIPTOR_H__\n> > \n> > #include \n> > +#include \n> > \n> > namespace libcamera {\n> > \n> > @@ -27,6 +28,8 @@ public:\n> > \tint fd() const { return fd_ ? fd_->fd() : -1; }\n> > \tFileDescriptor dup() const;\n> > \n> > +\tino_t inode() const;\n> > +\n> > private:\n> > \tclass Descriptor\n> > \t{\n> > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp\n> > index 9f9ebc81f738..d55e2b100b6d 100644\n> > --- a/src/libcamera/file_descriptor.cpp\n> > +++ b/src/libcamera/file_descriptor.cpp\n> > @@ -8,6 +8,8 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const\n> > \treturn FileDescriptor(fd());\n> > }\n> > \n> > +/**\n> > + * \\brief Retrieve the file descriptor inode\n> > + *\n> > + * \\todo Should this move to the File class ?\n> \n> I don't think so.\n> \n> File could implement an operator== which checks that the inode of both\n> is the same?\n> \n> Two File()'s with the same inode are the same, but two FileDescriptors\n> have ... separate file descriptors, so while they point to the same\n> location, they are not the same...\n\nIt depends how you define \"file\" :-) Calling open() twice on the same\npath (thus referring to the same inode) will create two files on the\nkernel side, each of them maintaining their own context (such as the\nread/write pointer and other flags).\n\nWe could compare File instances based on different underlying objects,\nincluding\n\n- The path (with or without following symlinks)\n- The kernel file instance (not sure how though, kcmp() may be an option)\n- The inode\n\nNote that inode comparison isn't enough, as they're not globally unique.\nThey are usually unique within a file system (so we can compare the stat\nst_dev and st_ino fields), but that's not always guaranteed (on btrfs\nfor instance, see https://lwn.net/Articles/866582/).\n\n> I guess a FileDescriptor is just a pointer, and the inode is the pointer\n> value ;-)\n\nThere are three levels, inode (within the context of a filesystem), file\n(from a kernel point of view) and file descriptor. You can have multiple\nfiles referring to the same inode (for instance by open()ing the same\npath twice), and multiple file descriptors referring to the same file\n(for instance by calling dup() or sending the file descriptor over IPC).\n\nSo FileDescriptor is indeed a pointer, but it's the top of the stack,\nwhile the inode is at the bottom.\n\n> > + *\n> > + * \\return The file descriptor inode on success, or 0 on error\n> > + */\n> > +ino_t FileDescriptor::inode() const\n> > +{\n> > +\tif (!fd_ || fd_->fd() == -1)\n> > +\t\treturn 0;\n> > +\n> > +\tstruct stat st;\n> > +\tint ret = fstat(fd_->fd(), &st);\n> > +\tif (ret < 0)\n> > +\t\treturn 0;\n> > +\n> > +\treturn st.st_ino;\n> \n> I'm torn ... we could parse this when we first construct, and cache the\n> inode.\n\nI didn't to avoid the overhead that is not required in the majority of\nuse cases.\n\n> That would save the system call on later comparisons, which may occur\n> more than once on some FileDescriptors, but ... never on others.\n> \n> \n> Or perhaps, as the inode can't be changed underneath the fd, it could\n> simply be cached on the first call ...\n\nI've thought about that too, but wasn't very keen on adding a field to\nthe FileDescriptor class for internal purpose only, as inode() isn't\nmeant to be a public API. If FileDescriptor was moved to base/ that may\nbe different, then we could consider which features belong to File and\nwhich belong to FileDescriptor, and create a nice API. For now, I've\ndecided to just add FileDescriptor::inode() without caching, and add a\n\\todo.\n\nOne option, if we decide to keep this in FileDescriptor, is to cache it\nin FileDescriptor::Descriptor.\n\n> Do you see any value in putting an operator== in here? It seems\n> reasonable to say that if two FileDescriptors have the same inode, they\n> are == ... however - they may of course be dup()s, with different FDs so\n> we might still want to distinguish that..\n\nI think an operator==() for both File and FileDescriptor would make\nsense, but again we need to define what equality means first :-) I'd\nrather not do so as part of this series, which I want to merge fast to\nfixes breakages in the master branch. That's why there's a todo at the\ntop of the patch, I think we can review the design of File and\nFileDescriptor later.\n\n> Anyway, a lot of that is dependant upon how it ends up getting used...\n> \n> I think it's a good thing to add.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +}\n> > +\n> > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> > {\n> > \tif (!duplicate) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D3A29BD87D\n\tfor ;\n\tThu, 2 Sep 2021 17:56:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 003016916A;\n\tThu, 2 Sep 2021 19:56:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9FC160255\n\tfor ;\n\tThu, 2 Sep 2021 19:56:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71F8C45E;\n\tThu, 2 Sep 2021 19:56:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gf0DZqSx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630605389;\n\tbh=YcqmFRMF3neydMosbDCl8CfKVRkA9SPmUkhuhqW6VgM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gf0DZqSxYfEdZeZPxAoEapC7oDbZg0mlYjBeYla5qi7QwGRl6C1dQ8rOz5yfw/oJ3\n\t0xOjbK25OLIC4yE7hcYCbGETM+pmzFhjBnuEw0T7dHEJz2/vKVILBm/QkyA1ZAt+il\n\tlI0E5dXTjZwHgUfsZPMQZFc2nShgNKkwG5+iqHsw=","Date":"Thu, 2 Sep 2021 20:56:12 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-3-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19342,"web_url":"https://patchwork.libcamera.org/comment/19342/","msgid":"","date":"2021-09-03T10:07:47","subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote:\n> > On 02/09/2021 05:22, Laurent Pinchart wrote:\n> > > The inode is useful to check if two file descriptors refer to the same\n> > > file. Add a function to retrieve it.\n> >\n> > Aha, great, and I expect this works on virtual-fs so dma-bufs will be\n> > comparable?\n>\n> That's the goal, yes.\n>\n> Dmabuf instances are associated with an inode, so two different file\n> descriptors that have the same inode are guaranteed to refer to the same\n> dmabuf (provided the file descriptors reference a dmabuf, inodes are not\n> unique globally). This works with dup() or when sending the file\n> descriptor over an IPC mechanism.\n>\n> > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media)\n>\n> For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer\n> will create multiple distinct dmabuf instances, and there's no way for\n> userspace to know that they related to the same underlying buffer.\n> Buffers allocated by DRM doesn't suffer from this issue, the always\n> return the same dmabuf when exporting the buffer multiple times. This\n> should be fixable on the V4L2 side.\n>\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/file_descriptor.h | 3 +++\n> > > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++\n> > > 2 files changed, 25 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h\n> > > index d514aac7697b..988f9b7a3d25 100644\n> > > --- a/include/libcamera/file_descriptor.h\n> > > +++ b/include/libcamera/file_descriptor.h\n> > > @@ -8,6 +8,7 @@\n> > > #define __LIBCAMERA_FILE_DESCRIPTOR_H__\n> > >\n> > > #include \n> > > +#include \n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -27,6 +28,8 @@ public:\n> > > int fd() const { return fd_ ? fd_->fd() : -1; }\n> > > FileDescriptor dup() const;\n> > >\n> > > + ino_t inode() const;\n> > > +\n> > > private:\n> > > class Descriptor\n> > > {\n> > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp\n> > > index 9f9ebc81f738..d55e2b100b6d 100644\n> > > --- a/src/libcamera/file_descriptor.cpp\n> > > +++ b/src/libcamera/file_descriptor.cpp\n> > > @@ -8,6 +8,8 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const\n> > > return FileDescriptor(fd());\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Retrieve the file descriptor inode\n> > > + *\n> > > + * \\todo Should this move to the File class ?\n> >\n> > I don't think so.\n> >\n> > File could implement an operator== which checks that the inode of both\n> > is the same?\n> >\n> > Two File()'s with the same inode are the same, but two FileDescriptors\n> > have ... separate file descriptors, so while they point to the same\n> > location, they are not the same...\n>\n> It depends how you define \"file\" :-) Calling open() twice on the same\n> path (thus referring to the same inode) will create two files on the\n> kernel side, each of them maintaining their own context (such as the\n> read/write pointer and other flags).\n>\n> We could compare File instances based on different underlying objects,\n> including\n>\n> - The path (with or without following symlinks)\n> - The kernel file instance (not sure how though, kcmp() may be an option)\n> - The inode\n>\n> Note that inode comparison isn't enough, as they're not globally unique.\n> They are usually unique within a file system (so we can compare the stat\n> st_dev and st_ino fields), but that's not always guaranteed (on btrfs\n> for instance, see https://lwn.net/Articles/866582/).\n>\n> > I guess a FileDescriptor is just a pointer, and the inode is the pointer\n> > value ;-)\n>\n> There are three levels, inode (within the context of a filesystem), file\n> (from a kernel point of view) and file descriptor. You can have multiple\n> files referring to the same inode (for instance by open()ing the same\n> path twice), and multiple file descriptors referring to the same file\n> (for instance by calling dup() or sending the file descriptor over IPC).\n>\n> So FileDescriptor is indeed a pointer, but it's the top of the stack,\n> while the inode is at the bottom.\n>\n> > > + *\n> > > + * \\return The file descriptor inode on success, or 0 on error\n> > > + */\n> > > +ino_t FileDescriptor::inode() const\n> > > +{\n> > > + if (!fd_ || fd_->fd() == -1)\n> > > + return 0;\n\nif (!isValid())\n return 0;\n\nAs far as I look the FileDescriptor code, if isValid() is true, fd_\nmust be valid.\n\n> > > +\n> > > + struct stat st;\n> > > + int ret = fstat(fd_->fd(), &st);\n> > > + if (ret < 0)\n> > > + return 0;\n\nShall we add the log?\nLOG(FileDescriptor, Error) << \"fstat failed: \" << errno;\n\n> > > +\n> > > + return st.st_ino;\n\nI am a bit concerned two invalid file descriptors are regarded thanks\nto returning 0.\nPerhaps, let the return type be int64_t and cast ino_t to int64_t?\nOr since ino_t is ulong_t=unsinged long=uint64_t (in 64bit machine),\nshall we return pair(ino_t, error) or error + pointer argument ino_t?\n\n> >\n> > I'm torn ... we could parse this when we first construct, and cache the\n> > inode.\n>\n> I didn't to avoid the overhead that is not required in the majority of\n> use cases.\n>\n> > That would save the system call on later comparisons, which may occur\n> > more than once on some FileDescriptors, but ... never on others.\n> >\n> >\n> > Or perhaps, as the inode can't be changed underneath the fd, it could\n> > simply be cached on the first call ...\n>\n> I've thought about that too, but wasn't very keen on adding a field to\n> the FileDescriptor class for internal purpose only, as inode() isn't\n> meant to be a public API. If FileDescriptor was moved to base/ that may\n> be different, then we could consider which features belong to File and\n> which belong to FileDescriptor, and create a nice API. For now, I've\n> decided to just add FileDescriptor::inode() without caching, and add a\n> \\todo.\n>\n> One option, if we decide to keep this in FileDescriptor, is to cache it\n> in FileDescriptor::Descriptor.\n>\n> > Do you see any value in putting an operator== in here? It seems\n> > reasonable to say that if two FileDescriptors have the same inode, they\n> > are == ... however - they may of course be dup()s, with different FDs so\n> > we might still want to distinguish that..\n>\n> I think an operator==() for both File and FileDescriptor would make\n> sense, but again we need to define what equality means first :-) I'd\n> rather not do so as part of this series, which I want to merge fast to\n> fixes breakages in the master branch. That's why there's a todo at the\n> top of the patch, I think we can review the design of File and\n> FileDescriptor later.\n>\n\nI could not imagine the use case of comparing File.\nFor FileDescriptors, it might be useful, I would use just inode\ncomparison manually on a caller side because currently the use cases\nare small enough (android/camera_device and libcamera/v4l2_device\nonly?).\n\nBest Regards,\n-Hiro\n> > Anyway, a lot of that is dependant upon how it ends up getting used...\n> >\n> > I think it's a good thing to add.\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > +}\n> > > +\n> > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> > > {\n> > > if (!duplicate) {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 58303BDC71\n\tfor ;\n\tFri, 3 Sep 2021 10:08:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB6F969168;\n\tFri, 3 Sep 2021 12:08:00 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3A5C69167\n\tfor ;\n\tFri, 3 Sep 2021 12:07:58 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id q3so7249549edt.5\n\tfor ;\n\tFri, 03 Sep 2021 03:07:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"GCwiTVR7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=lLY1vzntiKii1Dy0hCaFG07l2nmbafR27lFM4opC6D4=;\n\tb=GCwiTVR7lrUrdT+NGQQiuBrwknNU4OV9+FPGtxENPwDo7wXJ1z1zI4E79AESjkjWz0\n\t/qma8zlepNkNI4U1hJK/P/EVglJ35wa7CRsgAUZ06MjEy6Bh5Yrjk3YoFIhAX1mo5Prp\n\tHI2X7e8nHzM8hBBCvQGlmR0ty2srmmzJr/j/8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=lLY1vzntiKii1Dy0hCaFG07l2nmbafR27lFM4opC6D4=;\n\tb=RXwDn31GWTyhYbtYI4FpL1KiTM/HY8Hu7AZxrb7a9TptqZ2lsWNRaBOH2ycBlqmrqm\n\t99k2xgs0tnYt3RBTg5d/GOYErpgFTCi/WAuza4LJKN4oKO39NmSAJ44YOubx1+IawpSq\n\tP64sxxdt2iwrXgF3AG7sXD/MDKJM/j34vUc3cSSdJr7Jv/WkSNUq8lSgxT/WNPnTd6Jz\n\tyR28iaOriRnjVwyvhJ/PxSNW3R0l7ZD0MD5u2Yub9WtryVDXJctDPv61YRLOo3PPqRVl\n\tcXmxAZQ8M2qk0bEWJ6/3Tl9T+j6hDL29p/Nk7FtuW6kRjuu7bTsStGwN2nghx4OxrSLt\n\trlXw==","X-Gm-Message-State":"AOAM532w6xTzXIJ2oA8VCFbJ6+rXfjgcw5p2IWHvWwEN+iMSOv5mdVfl\n\t9VsIz2zdbP7GLefexzZvIGIRkvacw0rJmMT4egin8g==","X-Google-Smtp-Source":"ABdhPJwN31J7gveLwV4+HgX4iMuozA9sxieIzSJHNvulKuSskAfb/F5gl2s8TJ/mEI3dRosMP+LJSMPgelpe1bEWVqM=","X-Received":"by 2002:a05:6402:d7:: with SMTP id\n\ti23mr3130047edu.291.1630663678436; \n\tFri, 03 Sep 2021 03:07:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-3-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 3 Sep 2021 19:07:47 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19344,"web_url":"https://patchwork.libcamera.org/comment/19344/","msgid":"","date":"2021-09-03T10:34:47","subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Sep 03, 2021 at 07:07:47PM +0900, Hirokazu Honda wrote:\n> On Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart wrote:\n> > On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote:\n> > > On 02/09/2021 05:22, Laurent Pinchart wrote:\n> > > > The inode is useful to check if two file descriptors refer to the same\n> > > > file. Add a function to retrieve it.\n> > >\n> > > Aha, great, and I expect this works on virtual-fs so dma-bufs will be\n> > > comparable?\n> >\n> > That's the goal, yes.\n> >\n> > Dmabuf instances are associated with an inode, so two different file\n> > descriptors that have the same inode are guaranteed to refer to the same\n> > dmabuf (provided the file descriptors reference a dmabuf, inodes are not\n> > unique globally). This works with dup() or when sending the file\n> > descriptor over an IPC mechanism.\n> >\n> > > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media)\n> >\n> > For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer\n> > will create multiple distinct dmabuf instances, and there's no way for\n> > userspace to know that they related to the same underlying buffer.\n> > Buffers allocated by DRM doesn't suffer from this issue, the always\n> > return the same dmabuf when exporting the buffer multiple times. This\n> > should be fixable on the V4L2 side.\n> >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > include/libcamera/file_descriptor.h | 3 +++\n> > > > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++\n> > > > 2 files changed, 25 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h\n> > > > index d514aac7697b..988f9b7a3d25 100644\n> > > > --- a/include/libcamera/file_descriptor.h\n> > > > +++ b/include/libcamera/file_descriptor.h\n> > > > @@ -8,6 +8,7 @@\n> > > > #define __LIBCAMERA_FILE_DESCRIPTOR_H__\n> > > >\n> > > > #include \n> > > > +#include \n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > @@ -27,6 +28,8 @@ public:\n> > > > int fd() const { return fd_ ? fd_->fd() : -1; }\n> > > > FileDescriptor dup() const;\n> > > >\n> > > > + ino_t inode() const;\n> > > > +\n> > > > private:\n> > > > class Descriptor\n> > > > {\n> > > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp\n> > > > index 9f9ebc81f738..d55e2b100b6d 100644\n> > > > --- a/src/libcamera/file_descriptor.cpp\n> > > > +++ b/src/libcamera/file_descriptor.cpp\n> > > > @@ -8,6 +8,8 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const\n> > > > return FileDescriptor(fd());\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Retrieve the file descriptor inode\n> > > > + *\n> > > > + * \\todo Should this move to the File class ?\n> > >\n> > > I don't think so.\n> > >\n> > > File could implement an operator== which checks that the inode of both\n> > > is the same?\n> > >\n> > > Two File()'s with the same inode are the same, but two FileDescriptors\n> > > have ... separate file descriptors, so while they point to the same\n> > > location, they are not the same...\n> >\n> > It depends how you define \"file\" :-) Calling open() twice on the same\n> > path (thus referring to the same inode) will create two files on the\n> > kernel side, each of them maintaining their own context (such as the\n> > read/write pointer and other flags).\n> >\n> > We could compare File instances based on different underlying objects,\n> > including\n> >\n> > - The path (with or without following symlinks)\n> > - The kernel file instance (not sure how though, kcmp() may be an option)\n> > - The inode\n> >\n> > Note that inode comparison isn't enough, as they're not globally unique.\n> > They are usually unique within a file system (so we can compare the stat\n> > st_dev and st_ino fields), but that's not always guaranteed (on btrfs\n> > for instance, see https://lwn.net/Articles/866582/).\n> >\n> > > I guess a FileDescriptor is just a pointer, and the inode is the pointer\n> > > value ;-)\n> >\n> > There are three levels, inode (within the context of a filesystem), file\n> > (from a kernel point of view) and file descriptor. You can have multiple\n> > files referring to the same inode (for instance by open()ing the same\n> > path twice), and multiple file descriptors referring to the same file\n> > (for instance by calling dup() or sending the file descriptor over IPC).\n> >\n> > So FileDescriptor is indeed a pointer, but it's the top of the stack,\n> > while the inode is at the bottom.\n> >\n> > > > + *\n> > > > + * \\return The file descriptor inode on success, or 0 on error\n> > > > + */\n> > > > +ino_t FileDescriptor::inode() const\n> > > > +{\n> > > > + if (!fd_ || fd_->fd() == -1)\n> > > > + return 0;\n> \n> if (!isValid())\n> return 0;\n> \n> As far as I look the FileDescriptor code, if isValid() is true, fd_\n> must be valid.\n\nGood point, I'll do that.\n\n> > > > +\n> > > > + struct stat st;\n> > > > + int ret = fstat(fd_->fd(), &st);\n> > > > + if (ret < 0)\n> > > > + return 0;\n> \n> Shall we add the log?\n> LOG(FileDescriptor, Error) << \"fstat failed: \" << errno;\n\nI'll add an error message in v2.\n\n> > > > +\n> > > > + return st.st_ino;\n> \n> I am a bit concerned two invalid file descriptors are regarded thanks\n> to returning 0.\n> Perhaps, let the return type be int64_t and cast ino_t to int64_t?\n> Or since ino_t is ulong_t=unsinged long=uint64_t (in 64bit machine),\n> shall we return pair(ino_t, error) or error + pointer argument ino_t?\n\n0 is never allocated as a valid inode by filesystems, so I think it's\nsafe to use it to indicate an error. There are also a set of special\ninodes, for instance\n\n#define EXT4_BAD_INO 1 /* Bad blocks inode */\n#define EXT4_ROOT_INO 2 /* Root inode */\n#define EXT4_USR_QUOTA_INO 3 /* User quota inode */\n#define EXT4_GRP_QUOTA_INO 4 /* Group quota inode */\n#define EXT4_BOOT_LOADER_INO 5 /* Boot loader inode */\n#define EXT4_UNDEL_DIR_INO 6 /* Undelete directory inode */\n#define EXT4_RESIZE_INO 7 /* Reserved group descriptors inode */\n#define EXT4_JOURNAL_INO 8 /* Journal inode */\n\nbut that's filesystem-specific.\n\n> > >\n> > > I'm torn ... we could parse this when we first construct, and cache the\n> > > inode.\n> >\n> > I didn't to avoid the overhead that is not required in the majority of\n> > use cases.\n> >\n> > > That would save the system call on later comparisons, which may occur\n> > > more than once on some FileDescriptors, but ... never on others.\n> > >\n> > >\n> > > Or perhaps, as the inode can't be changed underneath the fd, it could\n> > > simply be cached on the first call ...\n> >\n> > I've thought about that too, but wasn't very keen on adding a field to\n> > the FileDescriptor class for internal purpose only, as inode() isn't\n> > meant to be a public API. If FileDescriptor was moved to base/ that may\n> > be different, then we could consider which features belong to File and\n> > which belong to FileDescriptor, and create a nice API. For now, I've\n> > decided to just add FileDescriptor::inode() without caching, and add a\n> > \\todo.\n> >\n> > One option, if we decide to keep this in FileDescriptor, is to cache it\n> > in FileDescriptor::Descriptor.\n> >\n> > > Do you see any value in putting an operator== in here? It seems\n> > > reasonable to say that if two FileDescriptors have the same inode, they\n> > > are == ... however - they may of course be dup()s, with different FDs so\n> > > we might still want to distinguish that..\n> >\n> > I think an operator==() for both File and FileDescriptor would make\n> > sense, but again we need to define what equality means first :-) I'd\n> > rather not do so as part of this series, which I want to merge fast to\n> > fixes breakages in the master branch. That's why there's a todo at the\n> > top of the patch, I think we can review the design of File and\n> > FileDescriptor later.\n> \n> I could not imagine the use case of comparing File.\n> For FileDescriptors, it might be useful, I would use just inode\n> comparison manually on a caller side because currently the use cases\n> are small enough (android/camera_device and libcamera/v4l2_device\n> only?).\n\nWe only have one user of the inode() function in this series, so it's\nindeed hard to know exactly how to design the API. I'd prefer waiting a\nbit for more use cases to appear before deciding on the semantics of the\nequality operator.\n\n> > > Anyway, a lot of that is dependant upon how it ends up getting used...\n> > >\n> > > I think it's a good thing to add.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > +}\n> > > > +\n> > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> > > > {\n> > > > if (!duplicate) {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3B289BD87D\n\tfor ;\n\tFri, 3 Sep 2021 10:35:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFFD26916A;\n\tFri, 3 Sep 2021 12:35:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F49A69167\n\tfor ;\n\tFri, 3 Sep 2021 12:35:05 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 19053BBE;\n\tFri, 3 Sep 2021 12:35:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AaiFKpGD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630665304;\n\tbh=cdr0RO73vRhSPnPxOlNcsAthOYKTE6nbMLyz+dqHIqo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AaiFKpGDkzM7rWuUl59TPacCPb+ePs69YtsP54f2qVaipPS9V1xzLZyNpCEZWICiN\n\tyjqalPo7PAcnwAGvgbAnvuu/1Qvk0n1WuDVWZDB9WW86zUi2663Rb21brpMCi0xV5Z\n\tGZuA7zfjHlEGWz/T4q6AVjwVeWCzZlIAvjxH1rXM=","Date":"Fri, 3 Sep 2021 13:34:47 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-3-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19346,"web_url":"https://patchwork.libcamera.org/comment/19346/","msgid":"","date":"2021-09-03T10:38:23","subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Fri, Sep 3, 2021 at 7:35 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Fri, Sep 03, 2021 at 07:07:47PM +0900, Hirokazu Honda wrote:\n> > On Fri, Sep 3, 2021 at 2:56 AM Laurent Pinchart wrote:\n> > > On Thu, Sep 02, 2021 at 09:39:11AM +0100, Kieran Bingham wrote:\n> > > > On 02/09/2021 05:22, Laurent Pinchart wrote:\n> > > > > The inode is useful to check if two file descriptors refer to the same\n> > > > > file. Add a function to retrieve it.\n> > > >\n> > > > Aha, great, and I expect this works on virtual-fs so dma-bufs will be\n> > > > comparable?\n> > >\n> > > That's the goal, yes.\n> > >\n> > > Dmabuf instances are associated with an inode, so two different file\n> > > descriptors that have the same inode are guaranteed to refer to the same\n> > > dmabuf (provided the file descriptors reference a dmabuf, inodes are not\n> > > unique globally). This works with dup() or when sending the file\n> > > descriptor over an IPC mechanism.\n> > >\n> > > > (edit: Except the VIDIOC_EXPBUF bug now being discussed on linux-media)\n> > >\n> > > For the interested reader: calling VIDIOC_EXPBUF on the same V4L2 buffer\n> > > will create multiple distinct dmabuf instances, and there's no way for\n> > > userspace to know that they related to the same underlying buffer.\n> > > Buffers allocated by DRM doesn't suffer from this issue, the always\n> > > return the same dmabuf when exporting the buffer multiple times. This\n> > > should be fixable on the V4L2 side.\n> > >\n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > include/libcamera/file_descriptor.h | 3 +++\n> > > > > src/libcamera/file_descriptor.cpp | 22 ++++++++++++++++++++++\n> > > > > 2 files changed, 25 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/file_descriptor.h b/include/libcamera/file_descriptor.h\n> > > > > index d514aac7697b..988f9b7a3d25 100644\n> > > > > --- a/include/libcamera/file_descriptor.h\n> > > > > +++ b/include/libcamera/file_descriptor.h\n> > > > > @@ -8,6 +8,7 @@\n> > > > > #define __LIBCAMERA_FILE_DESCRIPTOR_H__\n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > >\n> > > > > namespace libcamera {\n> > > > >\n> > > > > @@ -27,6 +28,8 @@ public:\n> > > > > int fd() const { return fd_ ? fd_->fd() : -1; }\n> > > > > FileDescriptor dup() const;\n> > > > >\n> > > > > + ino_t inode() const;\n> > > > > +\n> > > > > private:\n> > > > > class Descriptor\n> > > > > {\n> > > > > diff --git a/src/libcamera/file_descriptor.cpp b/src/libcamera/file_descriptor.cpp\n> > > > > index 9f9ebc81f738..d55e2b100b6d 100644\n> > > > > --- a/src/libcamera/file_descriptor.cpp\n> > > > > +++ b/src/libcamera/file_descriptor.cpp\n> > > > > @@ -8,6 +8,8 @@\n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > @@ -221,6 +223,26 @@ FileDescriptor FileDescriptor::dup() const\n> > > > > return FileDescriptor(fd());\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Retrieve the file descriptor inode\n> > > > > + *\n> > > > > + * \\todo Should this move to the File class ?\n> > > >\n> > > > I don't think so.\n> > > >\n> > > > File could implement an operator== which checks that the inode of both\n> > > > is the same?\n> > > >\n> > > > Two File()'s with the same inode are the same, but two FileDescriptors\n> > > > have ... separate file descriptors, so while they point to the same\n> > > > location, they are not the same...\n> > >\n> > > It depends how you define \"file\" :-) Calling open() twice on the same\n> > > path (thus referring to the same inode) will create two files on the\n> > > kernel side, each of them maintaining their own context (such as the\n> > > read/write pointer and other flags).\n> > >\n> > > We could compare File instances based on different underlying objects,\n> > > including\n> > >\n> > > - The path (with or without following symlinks)\n> > > - The kernel file instance (not sure how though, kcmp() may be an option)\n> > > - The inode\n> > >\n> > > Note that inode comparison isn't enough, as they're not globally unique.\n> > > They are usually unique within a file system (so we can compare the stat\n> > > st_dev and st_ino fields), but that's not always guaranteed (on btrfs\n> > > for instance, see https://lwn.net/Articles/866582/).\n> > >\n> > > > I guess a FileDescriptor is just a pointer, and the inode is the pointer\n> > > > value ;-)\n> > >\n> > > There are three levels, inode (within the context of a filesystem), file\n> > > (from a kernel point of view) and file descriptor. You can have multiple\n> > > files referring to the same inode (for instance by open()ing the same\n> > > path twice), and multiple file descriptors referring to the same file\n> > > (for instance by calling dup() or sending the file descriptor over IPC).\n> > >\n> > > So FileDescriptor is indeed a pointer, but it's the top of the stack,\n> > > while the inode is at the bottom.\n> > >\n> > > > > + *\n> > > > > + * \\return The file descriptor inode on success, or 0 on error\n> > > > > + */\n> > > > > +ino_t FileDescriptor::inode() const\n> > > > > +{\n> > > > > + if (!fd_ || fd_->fd() == -1)\n> > > > > + return 0;\n> >\n> > if (!isValid())\n> > return 0;\n> >\n> > As far as I look the FileDescriptor code, if isValid() is true, fd_\n> > must be valid.\n>\n> Good point, I'll do that.\n>\n> > > > > +\n> > > > > + struct stat st;\n> > > > > + int ret = fstat(fd_->fd(), &st);\n> > > > > + if (ret < 0)\n> > > > > + return 0;\n> >\n> > Shall we add the log?\n> > LOG(FileDescriptor, Error) << \"fstat failed: \" << errno;\n>\n> I'll add an error message in v2.\n>\n> > > > > +\n> > > > > + return st.st_ino;\n> >\n> > I am a bit concerned two invalid file descriptors are regarded thanks\n> > to returning 0.\n> > Perhaps, let the return type be int64_t and cast ino_t to int64_t?\n> > Or since ino_t is ulong_t=unsinged long=uint64_t (in 64bit machine),\n> > shall we return pair(ino_t, error) or error + pointer argument ino_t?\n>\n> 0 is never allocated as a valid inode by filesystems, so I think it's\n> safe to use it to indicate an error. There are also a set of special\n> inodes, for instance\n>\n> #define EXT4_BAD_INO 1 /* Bad blocks inode */\n> #define EXT4_ROOT_INO 2 /* Root inode */\n> #define EXT4_USR_QUOTA_INO 3 /* User quota inode */\n> #define EXT4_GRP_QUOTA_INO 4 /* Group quota inode */\n> #define EXT4_BOOT_LOADER_INO 5 /* Boot loader inode */\n> #define EXT4_UNDEL_DIR_INO 6 /* Undelete directory inode */\n> #define EXT4_RESIZE_INO 7 /* Reserved group descriptors inode */\n> #define EXT4_JOURNAL_INO 8 /* Journal inode */\n>\n> but that's filesystem-specific.\n>\n\nI got it.\n\n> > > >\n> > > > I'm torn ... we could parse this when we first construct, and cache the\n> > > > inode.\n> > >\n> > > I didn't to avoid the overhead that is not required in the majority of\n> > > use cases.\n> > >\n> > > > That would save the system call on later comparisons, which may occur\n> > > > more than once on some FileDescriptors, but ... never on others.\n> > > >\n> > > >\n> > > > Or perhaps, as the inode can't be changed underneath the fd, it could\n> > > > simply be cached on the first call ...\n> > >\n> > > I've thought about that too, but wasn't very keen on adding a field to\n> > > the FileDescriptor class for internal purpose only, as inode() isn't\n> > > meant to be a public API. If FileDescriptor was moved to base/ that may\n> > > be different, then we could consider which features belong to File and\n> > > which belong to FileDescriptor, and create a nice API. For now, I've\n> > > decided to just add FileDescriptor::inode() without caching, and add a\n> > > \\todo.\n> > >\n> > > One option, if we decide to keep this in FileDescriptor, is to cache it\n> > > in FileDescriptor::Descriptor.\n> > >\n> > > > Do you see any value in putting an operator== in here? It seems\n> > > > reasonable to say that if two FileDescriptors have the same inode, they\n> > > > are == ... however - they may of course be dup()s, with different FDs so\n> > > > we might still want to distinguish that..\n> > >\n> > > I think an operator==() for both File and FileDescriptor would make\n> > > sense, but again we need to define what equality means first :-) I'd\n> > > rather not do so as part of this series, which I want to merge fast to\n> > > fixes breakages in the master branch. That's why there's a todo at the\n> > > top of the patch, I think we can review the design of File and\n> > > FileDescriptor later.\n> >\n> > I could not imagine the use case of comparing File.\n> > For FileDescriptors, it might be useful, I would use just inode\n> > comparison manually on a caller side because currently the use cases\n> > are small enough (android/camera_device and libcamera/v4l2_device\n> > only?).\n>\n> We only have one user of the inode() function in this series, so it's\n> indeed hard to know exactly how to design the API. I'd prefer waiting a\n> bit for more use cases to appear before deciding on the semantics of the\n> equality operator.\n>\n\nYeah, I agree. If we want to add, we should start with defining\nequality and it shall be done later, not this patch series.\n\n-Hiro\n> > > > Anyway, a lot of that is dependant upon how it ends up getting used...\n> > > >\n> > > > I think it's a good thing to add.\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > > +}\n> > > > > +\n> > > > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> > > > > {\n> > > > > if (!duplicate) {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 400F0BDC71\n\tfor ;\n\tFri, 3 Sep 2021 10:38:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B056A6916D;\n\tFri, 3 Sep 2021 12:38:36 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F0DB69167\n\tfor ;\n\tFri, 3 Sep 2021 12:38:35 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id mf2so11145457ejb.9\n\tfor ;\n\tFri, 03 Sep 2021 03:38:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"cMye4KKg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=MaMPBMN6QZFsbjUpWXloj0RVNJDXga2/i3/baScZCgE=;\n\tb=cMye4KKgFAjAGnIsWxWuvHn9GV/g/LYW4RKmzrbM7DwOXVLzeCkcMkXJi0oG2SoLK8\n\tJ55jzHpJCjErRvaZ647fbcpmKGNKqzhFecRX07h916YlKPhokiIqzBsUdDRLbRsQubAi\n\t2Q2ijluhSgqdVyzKxDihmGxE8/6YgQRiAnh/k=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=MaMPBMN6QZFsbjUpWXloj0RVNJDXga2/i3/baScZCgE=;\n\tb=Mt+h7oYgJi3CEWR2o1+s8cpnP93JMLP9hK6JDANRkNEGQVBq5Kyx5vm4Ambj/WXosf\n\t9AHUfxxNepXFksjXome+BNZLZvJofsXbwhFYa47OpMPlzCUNDSLQ5y0MDQi1p4gg/Oy8\n\tehKzKKkvmY9r8hsWlY5Od1drGseq7EWKYXuaEY3mNmcRuOi9SByDbIwqJqLU4GYP/5Ap\n\tvVAis42POylHOhFWQed0EArjhrPSm2Bt3OFv51PBJfn91PeD8EG5Yym1b436pKGCQb+c\n\tw2TdmoFQlqmD1MjYNm4RNbUS2jds4y+IW5Gj29lfUUoG5Ih+bAsxQhRWbbIkf8exuQhf\n\tRyyA==","X-Gm-Message-State":"AOAM5334xsCIcUKVqX3BbmUM0ZVNdaT7YBYAsiPSd2TEClszregd1ZGQ\n\tjiplwbmFGIlRgaObCUPi8DVjfMpiTUFGXnhyS/d8Hvvj36M=","X-Google-Smtp-Source":"ABdhPJwEQ97kvxwG+AofqrNMbFlqWSALm0fwCKeU2a6KwdTO5XSfr4BzV9h9UdmOZcJ2MDe60/lrOIHXDM3qn39RgBY=","X-Received":"by 2002:a17:906:32c9:: with SMTP id\n\tk9mr3500410ejk.218.1630665514987; \n\tFri, 03 Sep 2021 03:38:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-3-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 3 Sep 2021 19:38:23 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v1 02/12] libcamera:\n\tfile_descriptor: Add a function to retrieve the inode","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]