[{"id":21316,"web_url":"https://patchwork.libcamera.org/comment/21316/","msgid":"","date":"2021-11-29T13:09:43","subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","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 Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart\n wrote:\n>\n> The inode() function has always been a bit of an outcast in the\n> FileDescriptor class, as it's not related to the core feature provided\n> by FileDescriptor, a shared ownership wrapper around file descriptors.\n> Move it to the File class instead. It may not be a perfect fit there\n> either, but File is a private class, so the inode() function is at least\n> not part of the public API anymore.\n>\n> Signed-off-by: Laurent Pinchart \n\nI don't have a strong opinion here. But inode() looks to be called in\nframe_buffer.cpp, should it be put in frame_buffer.cpp?\n\nBest,\n-Hiro\n> ---\n> include/libcamera/base/file.h | 3 +++\n> include/libcamera/base/file_descriptor.h | 3 ---\n> src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++\n> src/libcamera/base/file_descriptor.cpp | 25 ------------------------\n> src/libcamera/framebuffer.cpp | 5 +++--\n> 5 files changed, 29 insertions(+), 30 deletions(-)\n>\n> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> index 55e8edd934d4..996751a7ab72 100644\n> --- a/include/libcamera/base/file.h\n> +++ b/include/libcamera/base/file.h\n> @@ -20,6 +20,8 @@\n>\n> namespace libcamera {\n>\n> +class FileDescriptor;\n> +\n> class File\n> {\n> public:\n> @@ -66,6 +68,7 @@ public:\n> bool unmap(uint8_t *addr);\n>\n> static bool exists(const std::string &name);\n> + static ino_t inode(const FileDescriptor &fd);\n>\n> private:\n> LIBCAMERA_DISABLE_COPY(File)\n> diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h\n> index 8d764f8b4a26..5d1594e80801 100644\n> --- a/include/libcamera/base/file_descriptor.h\n> +++ b/include/libcamera/base/file_descriptor.h\n> @@ -8,7 +8,6 @@\n> #pragma once\n>\n> #include \n> -#include \n>\n> namespace libcamera {\n>\n> @@ -28,8 +27,6 @@ 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/base/file.cpp b/src/libcamera/base/file.cpp\n> index ae4be1f95fa6..7043f9461cf7 100644\n> --- a/src/libcamera/base/file.cpp\n> +++ b/src/libcamera/base/file.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n>\n> +#include \n> #include \n>\n> /**\n> @@ -472,4 +473,26 @@ bool File::exists(const std::string &name)\n> return !S_ISDIR(st.st_mode);\n> }\n>\n> +/**\n> + * \\brief Retrieve the inode of a FileDescriptor\n> + *\n> + * \\return The file descriptor inode on success, or 0 on error\n> + */\n> +ino_t File::inode(const FileDescriptor &fd)\n> +{\n> + if (!fd.isValid())\n> + return 0;\n> +\n> + struct stat st;\n> + int ret = fstat(fd.fd(), &st);\n> + if (ret < 0) {\n> + ret = -errno;\n> + LOG(File, Fatal)\n> + << \"Failed to fstat() fd: \" << strerror(-ret);\n> + return 0;\n> + }\n> +\n> + return st.st_ino;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp\n> index f5f87c56eee8..98d4b4bfd24f 100644\n> --- a/src/libcamera/base/file_descriptor.cpp\n> +++ b/src/libcamera/base/file_descriptor.cpp\n> @@ -8,7 +8,6 @@\n> #include \n>\n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -223,30 +222,6 @@ 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> - * \\return The file descriptor inode on success, or 0 on error\n> - */\n> -ino_t FileDescriptor::inode() const\n> -{\n> - if (!isValid())\n> - return 0;\n> -\n> - struct stat st;\n> - int ret = fstat(fd_->fd(), &st);\n> - if (ret < 0) {\n> - ret = -errno;\n> - LOG(FileDescriptor, Fatal)\n> - << \"Failed to fstat() fd: \" << strerror(-ret);\n> - return 0;\n> - }\n> -\n> - return st.st_ino;\n> -}\n> -\n> FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> {\n> if (!duplicate) {\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 337ea1155a38..f5bcf107d7aa 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> #include \"libcamera/internal/framebuffer.h\"\n>\n> +#include \n> #include \n>\n> /**\n> @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> */\n> if (plane.fd.fd() != planes_[0].fd.fd()) {\n> if (!inode)\n> - inode = planes_[0].fd.inode();\n> - if (plane.fd.inode() != inode) {\n> + inode = File::inode(planes_[0].fd);\n> + if (File::inode(plane.fd) != inode) {\n> isContiguous = false;\n> break;\n> }\n> --\n> Regards,\n>\n> Laurent Pinchart\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 3F6FBBF415\n\tfor ;\n\tMon, 29 Nov 2021 13:09:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89590605A4;\n\tMon, 29 Nov 2021 14:09:57 +0100 (CET)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B904760592\n\tfor ;\n\tMon, 29 Nov 2021 14:09:55 +0100 (CET)","by mail-ed1-x533.google.com with SMTP id y13so71529395edd.13\n\tfor ;\n\tMon, 29 Nov 2021 05:09:55 -0800 (PST)"],"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=\"beaYB0b6\"; 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=QniL7bKIjOAlTTdg87VxwGzssfEitZeQm7lAUlhYUUY=;\n\tb=beaYB0b6+9RQ2LaaHkxLd/et7/K7BOKM7JAnOUXkgQzQo2UubY7qyCx31zY5QEOkgu\n\twn5u8+W9PA/RvageaTr2wnfAAUAK9c1JueqUqNbpIH4/4cVH8yczZWK9A6REYbupx44I\n\tPQhD/wno+fLAYFoXaTxIIafDhXXGnQ1Cd7SpA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=QniL7bKIjOAlTTdg87VxwGzssfEitZeQm7lAUlhYUUY=;\n\tb=FRam49Yno04QPO91mIyQdW7vfARbMwy87b+VAtnka2cmBNOzfhzFRGbDnqdm74KOR3\n\tfNnUBG9o/Ov6A/2jw4hz47em4I4BFcTVbbMFPiw98pblTOtvLgZRFdE2r/NLh8LRZJm9\n\tuw26ljPqFGqUExlNfSTUL4uxct4RBV7jP83lF4aBZrY//Ko+QPA53YgJ8xrAAeTNEIcR\n\tiIV1tAaOCfIHMBJVjz4vHRvDHVfxmpBjYV3E+5MkwXmzAiMYlRLXEna94KkjRKGp2jKw\n\tTt8Y7py4wiipqn2G0zfhev8SR/CIkG2lWZ+VC3ASEdyRQs1rXfqlbrP0TnzwulenrNQ7\n\thlEg==","X-Gm-Message-State":"AOAM533Vgn3jPh0We7zbhTtERVocry3hZMwHUsNoy5EBzj4UzMF/jt33\n\tJtslIogzZOhX7UFo9w6LJbBFly+/8mIseEZuplYd4wi4ac8=","X-Google-Smtp-Source":"ABdhPJx2OB/nM7bDdwcGS58KlDuiZ/pKLLJEN0MrSrqO0wE1AzR4Wtj7V36uhtFi0EUldD+xu9bUyiqUcuTGWH02ih0=","X-Received":"by 2002:a05:6402:2552:: with SMTP id\n\tl18mr74548352edb.368.1638191395304; \n\tMon, 29 Nov 2021 05:09:55 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-4-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211128235752.10836-4-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 22:09:43 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","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":21330,"web_url":"https://patchwork.libcamera.org/comment/21330/","msgid":"<20211129143213.v5cbkd3kxkcui456@uno.localdomain>","date":"2021-11-29T14:32:13","subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Nov 29, 2021 at 01:57:38AM +0200, Laurent Pinchart wrote:\n> The inode() function has always been a bit of an outcast in the\n> FileDescriptor class, as it's not related to the core feature provided\n> by FileDescriptor, a shared ownership wrapper around file descriptors.\n> Move it to the File class instead. It may not be a perfect fit there\n> either, but File is a private class, so the inode() function is at least\n> not part of the public API anymore.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/file.h | 3 +++\n> include/libcamera/base/file_descriptor.h | 3 ---\n> src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++\n> src/libcamera/base/file_descriptor.cpp | 25 ------------------------\n> src/libcamera/framebuffer.cpp | 5 +++--\n> 5 files changed, 29 insertions(+), 30 deletions(-)\n>\n> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> index 55e8edd934d4..996751a7ab72 100644\n> --- a/include/libcamera/base/file.h\n> +++ b/include/libcamera/base/file.h\n> @@ -20,6 +20,8 @@\n>\n> namespace libcamera {\n>\n> +class FileDescriptor;\n> +\n> class File\n> {\n> public:\n> @@ -66,6 +68,7 @@ public:\n> \tbool unmap(uint8_t *addr);\n>\n> \tstatic bool exists(const std::string &name);\n> +\tstatic ino_t inode(const FileDescriptor &fd);\n>\n> private:\n> \tLIBCAMERA_DISABLE_COPY(File)\n> diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h\n> index 8d764f8b4a26..5d1594e80801 100644\n> --- a/include/libcamera/base/file_descriptor.h\n> +++ b/include/libcamera/base/file_descriptor.h\n> @@ -8,7 +8,6 @@\n> #pragma once\n>\n> #include \n> -#include \n>\n> namespace libcamera {\n>\n> @@ -28,8 +27,6 @@ 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/base/file.cpp b/src/libcamera/base/file.cpp\n> index ae4be1f95fa6..7043f9461cf7 100644\n> --- a/src/libcamera/base/file.cpp\n> +++ b/src/libcamera/base/file.cpp\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n>\n> +#include \n> #include \n>\n> /**\n> @@ -472,4 +473,26 @@ bool File::exists(const std::string &name)\n> \treturn !S_ISDIR(st.st_mode);\n> }\n>\n> +/**\n> + * \\brief Retrieve the inode of a FileDescriptor\n> + *\n> + * \\return The file descriptor inode on success, or 0 on error\n> + */\n> +ino_t File::inode(const FileDescriptor &fd)\n> +{\n> +\tif (!fd.isValid())\n> +\t\treturn 0;\n> +\n> +\tstruct stat st;\n> +\tint ret = fstat(fd.fd(), &st);\n> +\tif (ret < 0) {\n> +\t\tret = -errno;\n> +\t\tLOG(File, Fatal)\n> +\t\t\t<< \"Failed to fstat() fd: \" << strerror(-ret);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn st.st_ino;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp\n> index f5f87c56eee8..98d4b4bfd24f 100644\n> --- a/src/libcamera/base/file_descriptor.cpp\n> +++ b/src/libcamera/base/file_descriptor.cpp\n> @@ -8,7 +8,6 @@\n> #include \n>\n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -223,30 +222,6 @@ 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> - * \\return The file descriptor inode on success, or 0 on error\n> - */\n> -ino_t FileDescriptor::inode() const\n> -{\n> -\tif (!isValid())\n> -\t\treturn 0;\n> -\n> -\tstruct stat st;\n> -\tint ret = fstat(fd_->fd(), &st);\n> -\tif (ret < 0) {\n> -\t\tret = -errno;\n> -\t\tLOG(FileDescriptor, Fatal)\n> -\t\t\t<< \"Failed to fstat() fd: \" << strerror(-ret);\n> -\t\treturn 0;\n> -\t}\n> -\n> -\treturn st.st_ino;\n> -}\n> -\n> FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> {\n> \tif (!duplicate) {\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 337ea1155a38..f5bcf107d7aa 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> #include \"libcamera/internal/framebuffer.h\"\n>\n> +#include \n> #include \n>\n> /**\n> @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> \t\t */\n> \t\tif (plane.fd.fd() != planes_[0].fd.fd()) {\n> \t\t\tif (!inode)\n> -\t\t\t\tinode = planes_[0].fd.inode();\n> -\t\t\tif (plane.fd.inode() != inode) {\n> +\t\t\t\tinode = File::inode(planes_[0].fd);\n> +\t\t\tif (File::inode(plane.fd) != inode) {\n\nAs that's only user I'm not sure I get why a static function is better :/\n\n\n> \t\t\t\tisContiguous = false;\n> \t\t\t\tbreak;\n> \t\t\t}\n> --\n> Regards,\n>\n> Laurent Pinchart\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 8C257BF415\n\tfor ;\n\tMon, 29 Nov 2021 14:31:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6BCC605A4;\n\tMon, 29 Nov 2021 15:31:21 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3557760592\n\tfor ;\n\tMon, 29 Nov 2021 15:31:21 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B239240008;\n\tMon, 29 Nov 2021 14:31:20 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 15:32:13 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129143213.v5cbkd3kxkcui456@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211128235752.10836-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","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":21352,"web_url":"https://patchwork.libcamera.org/comment/21352/","msgid":"","date":"2021-11-29T16:34:41","subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Nov 29, 2021 at 03:32:13PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 29, 2021 at 01:57:38AM +0200, Laurent Pinchart wrote:\n> > The inode() function has always been a bit of an outcast in the\n> > FileDescriptor class, as it's not related to the core feature provided\n> > by FileDescriptor, a shared ownership wrapper around file descriptors.\n> > Move it to the File class instead. It may not be a perfect fit there\n> > either, but File is a private class, so the inode() function is at least\n> > not part of the public API anymore.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/file.h | 3 +++\n> > include/libcamera/base/file_descriptor.h | 3 ---\n> > src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++\n> > src/libcamera/base/file_descriptor.cpp | 25 ------------------------\n> > src/libcamera/framebuffer.cpp | 5 +++--\n> > 5 files changed, 29 insertions(+), 30 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > index 55e8edd934d4..996751a7ab72 100644\n> > --- a/include/libcamera/base/file.h\n> > +++ b/include/libcamera/base/file.h\n> > @@ -20,6 +20,8 @@\n> >\n> > namespace libcamera {\n> >\n> > +class FileDescriptor;\n> > +\n> > class File\n> > {\n> > public:\n> > @@ -66,6 +68,7 @@ public:\n> > \tbool unmap(uint8_t *addr);\n> >\n> > \tstatic bool exists(const std::string &name);\n> > +\tstatic ino_t inode(const FileDescriptor &fd);\n> >\n> > private:\n> > \tLIBCAMERA_DISABLE_COPY(File)\n> > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h\n> > index 8d764f8b4a26..5d1594e80801 100644\n> > --- a/include/libcamera/base/file_descriptor.h\n> > +++ b/include/libcamera/base/file_descriptor.h\n> > @@ -8,7 +8,6 @@\n> > #pragma once\n> >\n> > #include \n> > -#include \n> >\n> > namespace libcamera {\n> >\n> > @@ -28,8 +27,6 @@ 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/base/file.cpp b/src/libcamera/base/file.cpp\n> > index ae4be1f95fa6..7043f9461cf7 100644\n> > --- a/src/libcamera/base/file.cpp\n> > +++ b/src/libcamera/base/file.cpp\n> > @@ -14,6 +14,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > /**\n> > @@ -472,4 +473,26 @@ bool File::exists(const std::string &name)\n> > \treturn !S_ISDIR(st.st_mode);\n> > }\n> >\n> > +/**\n> > + * \\brief Retrieve the inode of a FileDescriptor\n> > + *\n> > + * \\return The file descriptor inode on success, or 0 on error\n> > + */\n> > +ino_t File::inode(const FileDescriptor &fd)\n> > +{\n> > +\tif (!fd.isValid())\n> > +\t\treturn 0;\n> > +\n> > +\tstruct stat st;\n> > +\tint ret = fstat(fd.fd(), &st);\n> > +\tif (ret < 0) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(File, Fatal)\n> > +\t\t\t<< \"Failed to fstat() fd: \" << strerror(-ret);\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\treturn st.st_ino;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp\n> > index f5f87c56eee8..98d4b4bfd24f 100644\n> > --- a/src/libcamera/base/file_descriptor.cpp\n> > +++ b/src/libcamera/base/file_descriptor.cpp\n> > @@ -8,7 +8,6 @@\n> > #include \n> >\n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -223,30 +222,6 @@ 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> > - * \\return The file descriptor inode on success, or 0 on error\n> > - */\n> > -ino_t FileDescriptor::inode() const\n> > -{\n> > -\tif (!isValid())\n> > -\t\treturn 0;\n> > -\n> > -\tstruct stat st;\n> > -\tint ret = fstat(fd_->fd(), &st);\n> > -\tif (ret < 0) {\n> > -\t\tret = -errno;\n> > -\t\tLOG(FileDescriptor, Fatal)\n> > -\t\t\t<< \"Failed to fstat() fd: \" << strerror(-ret);\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> > -\treturn st.st_ino;\n> > -}\n> > -\n> > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> > {\n> > \tif (!duplicate) {\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index 337ea1155a38..f5bcf107d7aa 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> > #include \"libcamera/internal/framebuffer.h\"\n> >\n> > +#include \n> > #include \n> >\n> > /**\n> > @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > \t\t */\n> > \t\tif (plane.fd.fd() != planes_[0].fd.fd()) {\n> > \t\t\tif (!inode)\n> > -\t\t\t\tinode = planes_[0].fd.inode();\n> > -\t\t\tif (plane.fd.inode() != inode) {\n> > +\t\t\t\tinode = File::inode(planes_[0].fd);\n> > +\t\t\tif (File::inode(plane.fd) != inode) {\n> \n> As that's only user I'm not sure I get why a static function is better :/\n\nI've considered that, and then thought the function may be useful\nsomewhere else in the future. As both Hiro and you have the same\ncomment, I'll move it as a static function in this file.\n\n> > \t\t\t\tisContiguous = false;\n> > \t\t\t\tbreak;\n> > \t\t\t}","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 4C78DBDB13\n\tfor ;\n\tMon, 29 Nov 2021 16:35:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BC04605A4;\n\tMon, 29 Nov 2021 17:35:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D62360592\n\tfor ;\n\tMon, 29 Nov 2021 17:35:05 +0100 (CET)","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 0A1302A5;\n\tMon, 29 Nov 2021 17:35:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JobnAODY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638203705;\n\tbh=acczODztska6TZCDCSQT0YmA/N1ww6FRpGBvcdp1X2c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JobnAODYYvs1Bi1T8RRI9Ud1ZzILFpD+Ylk3cEEEo2fmb7mdoR7vwiaYJuvJyVaAI\n\tu8Iq//zmKwd+mKRxRPMaX/lNam+uOSzKPQ4VFLsZK+f/pRzmYF46RBuX0o+MH0d6xA\n\tpOwKzudJW3M0JSJ6KjM2/fMnhltnl8m7bt0dGOvU=","Date":"Mon, 29 Nov 2021 18:34:41 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-4-laurent.pinchart@ideasonboard.com>\n\t<20211129143213.v5cbkd3kxkcui456@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211129143213.v5cbkd3kxkcui456@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","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":21360,"web_url":"https://patchwork.libcamera.org/comment/21360/","msgid":"<20211129170443.42zycyrzbzozsx3s@uno.localdomain>","date":"2021-11-29T17:04:43","subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Nov 29, 2021 at 06:34:41PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Nov 29, 2021 at 03:32:13PM +0100, Jacopo Mondi wrote:\n> > On Mon, Nov 29, 2021 at 01:57:38AM +0200, Laurent Pinchart wrote:\n> > > The inode() function has always been a bit of an outcast in the\n> > > FileDescriptor class, as it's not related to the core feature provided\n> > > by FileDescriptor, a shared ownership wrapper around file descriptors.\n> > > Move it to the File class instead. It may not be a perfect fit there\n> > > either, but File is a private class, so the inode() function is at least\n> > > not part of the public API anymore.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/base/file.h | 3 +++\n> > > include/libcamera/base/file_descriptor.h | 3 ---\n> > > src/libcamera/base/file.cpp | 23 ++++++++++++++++++++++\n> > > src/libcamera/base/file_descriptor.cpp | 25 ------------------------\n> > > src/libcamera/framebuffer.cpp | 5 +++--\n> > > 5 files changed, 29 insertions(+), 30 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > > index 55e8edd934d4..996751a7ab72 100644\n> > > --- a/include/libcamera/base/file.h\n> > > +++ b/include/libcamera/base/file.h\n> > > @@ -20,6 +20,8 @@\n> > >\n> > > namespace libcamera {\n> > >\n> > > +class FileDescriptor;\n> > > +\n> > > class File\n> > > {\n> > > public:\n> > > @@ -66,6 +68,7 @@ public:\n> > > \tbool unmap(uint8_t *addr);\n> > >\n> > > \tstatic bool exists(const std::string &name);\n> > > +\tstatic ino_t inode(const FileDescriptor &fd);\n> > >\n> > > private:\n> > > \tLIBCAMERA_DISABLE_COPY(File)\n> > > diff --git a/include/libcamera/base/file_descriptor.h b/include/libcamera/base/file_descriptor.h\n> > > index 8d764f8b4a26..5d1594e80801 100644\n> > > --- a/include/libcamera/base/file_descriptor.h\n> > > +++ b/include/libcamera/base/file_descriptor.h\n> > > @@ -8,7 +8,6 @@\n> > > #pragma once\n> > >\n> > > #include \n> > > -#include \n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -28,8 +27,6 @@ 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/base/file.cpp b/src/libcamera/base/file.cpp\n> > > index ae4be1f95fa6..7043f9461cf7 100644\n> > > --- a/src/libcamera/base/file.cpp\n> > > +++ b/src/libcamera/base/file.cpp\n> > > @@ -14,6 +14,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > /**\n> > > @@ -472,4 +473,26 @@ bool File::exists(const std::string &name)\n> > > \treturn !S_ISDIR(st.st_mode);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Retrieve the inode of a FileDescriptor\n> > > + *\n> > > + * \\return The file descriptor inode on success, or 0 on error\n> > > + */\n> > > +ino_t File::inode(const FileDescriptor &fd)\n> > > +{\n> > > +\tif (!fd.isValid())\n> > > +\t\treturn 0;\n> > > +\n> > > +\tstruct stat st;\n> > > +\tint ret = fstat(fd.fd(), &st);\n> > > +\tif (ret < 0) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(File, Fatal)\n> > > +\t\t\t<< \"Failed to fstat() fd: \" << strerror(-ret);\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\treturn st.st_ino;\n> > > +}\n> > > +\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/base/file_descriptor.cpp b/src/libcamera/base/file_descriptor.cpp\n> > > index f5f87c56eee8..98d4b4bfd24f 100644\n> > > --- a/src/libcamera/base/file_descriptor.cpp\n> > > +++ b/src/libcamera/base/file_descriptor.cpp\n> > > @@ -8,7 +8,6 @@\n> > > #include \n> > >\n> > > #include \n> > > -#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -223,30 +222,6 @@ 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> > > - * \\return The file descriptor inode on success, or 0 on error\n> > > - */\n> > > -ino_t FileDescriptor::inode() const\n> > > -{\n> > > -\tif (!isValid())\n> > > -\t\treturn 0;\n> > > -\n> > > -\tstruct stat st;\n> > > -\tint ret = fstat(fd_->fd(), &st);\n> > > -\tif (ret < 0) {\n> > > -\t\tret = -errno;\n> > > -\t\tLOG(FileDescriptor, Fatal)\n> > > -\t\t\t<< \"Failed to fstat() fd: \" << strerror(-ret);\n> > > -\t\treturn 0;\n> > > -\t}\n> > > -\n> > > -\treturn st.st_ino;\n> > > -}\n> > > -\n> > > FileDescriptor::Descriptor::Descriptor(int fd, bool duplicate)\n> > > {\n> > > \tif (!duplicate) {\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index 337ea1155a38..f5bcf107d7aa 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \n> > > #include \"libcamera/internal/framebuffer.h\"\n> > >\n> > > +#include \n> > > #include \n> > >\n> > > /**\n> > > @@ -236,8 +237,8 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > \t\t */\n> > > \t\tif (plane.fd.fd() != planes_[0].fd.fd()) {\n> > > \t\t\tif (!inode)\n> > > -\t\t\t\tinode = planes_[0].fd.inode();\n> > > -\t\t\tif (plane.fd.inode() != inode) {\n> > > +\t\t\t\tinode = File::inode(planes_[0].fd);\n> > > +\t\t\tif (File::inode(plane.fd) != inode) {\n> >\n> > As that's only user I'm not sure I get why a static function is better :/\n>\n> I've considered that, and then thought the function may be useful\n> somewhere else in the future. As both Hiro and you have the same\n> comment, I'll move it as a static function in this file.\n>\n\nI was suggesting to keep it in FileDescriptor, but since we have two\nfile descriptors now I understand it's not a good idea. Move it\nwherever is better !\n\n> > > \t\t\t\tisContiguous = false;\n> > > \t\t\t\tbreak;\n> > > \t\t\t}\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 96BD5BF415\n\tfor ;\n\tMon, 29 Nov 2021 17:03:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D97C605A5;\n\tMon, 29 Nov 2021 18:03:52 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15B1C60592\n\tfor ;\n\tMon, 29 Nov 2021 18:03:51 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 9E420C001D;\n\tMon, 29 Nov 2021 17:03:50 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 18:04:43 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129170443.42zycyrzbzozsx3s@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-4-laurent.pinchart@ideasonboard.com>\n\t<20211129143213.v5cbkd3kxkcui456@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 03/17] libcamera: base:\n\tfile_descriptor: Move inode() function to File class","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\" "}}]