[{"id":21320,"web_url":"https://patchwork.libcamera.org/comment/21320/","msgid":"","date":"2021-11-29T13:54:12","subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart\n wrote:\n>\n> From: Hirokazu Honda \n>\n> Manages the file descriptor owned by File by UniqueFD.\n>\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/base/file.h | 5 +++--\n> src/libcamera/base/file.cpp | 25 ++++++++++++-------------\n> 2 files changed, 15 insertions(+), 15 deletions(-)\n>\n> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> index 996751a7ab72..47769da7abc2 100644\n> --- a/include/libcamera/base/file.h\n> +++ b/include/libcamera/base/file.h\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n>\n> namespace libcamera {\n>\n> @@ -50,7 +51,7 @@ public:\n> bool exists() const;\n>\n> bool open(OpenMode mode);\n> - bool isOpen() const { return fd_ != -1; }\n> + bool isOpen() const { return fd_.isValid(); }\n> OpenMode openMode() const { return mode_; }\n> void close();\n>\n> @@ -76,7 +77,7 @@ private:\n> void unmapAll();\n>\n> std::string name_;\n> - int fd_;\n> + UniqueFD fd_;\n> OpenMode mode_;\n>\n> int error_;\n> diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> index 7043f9461cf7..66c73c406198 100644\n> --- a/src/libcamera/base/file.cpp\n> +++ b/src/libcamera/base/file.cpp\n> @@ -84,7 +84,7 @@ LOG_DEFINE_CATEGORY(File)\n> * before performing I/O operations.\n> */\n> File::File(const std::string &name)\n> - : name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> + : name_(name), mode_(OpenModeFlag::NotOpen), error_(0)\n> {\n> }\n>\n> @@ -95,7 +95,7 @@ File::File(const std::string &name)\n> * setFileName().\n> */\n> File::File()\n> - : fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> + : mode_(OpenModeFlag::NotOpen), error_(0)\n> {\n> }\n>\n> @@ -178,8 +178,8 @@ bool File::open(File::OpenMode mode)\n> if (mode & OpenModeFlag::WriteOnly)\n> flags |= O_CREAT;\n>\n> - fd_ = ::open(name_.c_str(), flags, 0666);\n> - if (fd_ < 0) {\n> + fd_ = UniqueFD(::open(name_.c_str(), flags, 0666));\n\nThis is not quite related to this patch series.\nChrome uses HANDLE_EINTR macro [1] to retry system call in the case of EINTR.\nIt is often used with dup and open call. Shall we use it in libcamera?\n\n[1] https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=3553913abdd97123c3937277f26cba44e6eacacf\n\n-Hiro\n\n> + if (!fd_.isValid()) {\n> error_ = -errno;\n> return false;\n> }\n> @@ -210,11 +210,10 @@ bool File::open(File::OpenMode mode)\n> */\n> void File::close()\n> {\n> - if (fd_ == -1)\n> + if (!fd_.isValid())\n> return;\n>\n> - ::close(fd_);\n> - fd_ = -1;\n> + fd_.reset();\n> mode_ = OpenModeFlag::NotOpen;\n> }\n>\n> @@ -244,7 +243,7 @@ ssize_t File::size() const\n> return -EINVAL;\n>\n> struct stat st;\n> - int ret = fstat(fd_, &st);\n> + int ret = fstat(fd_.get(), &st);\n> if (ret < 0)\n> return -errno;\n>\n> @@ -263,7 +262,7 @@ off_t File::pos() const\n> if (!isOpen())\n> return 0;\n>\n> - return lseek(fd_, 0, SEEK_CUR);\n> + return lseek(fd_.get(), 0, SEEK_CUR);\n> }\n>\n> /**\n> @@ -277,7 +276,7 @@ off_t File::seek(off_t pos)\n> if (!isOpen())\n> return -EINVAL;\n>\n> - off_t ret = lseek(fd_, pos, SEEK_SET);\n> + off_t ret = lseek(fd_.get(), pos, SEEK_SET);\n> if (ret < 0)\n> return -errno;\n>\n> @@ -309,7 +308,7 @@ ssize_t File::read(const Span &data)\n>\n> /* Retry in case of interrupted system calls. */\n> while (readBytes < data.size()) {\n> - ret = ::read(fd_, data.data() + readBytes,\n> + ret = ::read(fd_.get(), data.data() + readBytes,\n> data.size() - readBytes);\n> if (ret <= 0)\n> break;\n> @@ -346,7 +345,7 @@ ssize_t File::write(const Span &data)\n>\n> /* Retry in case of interrupted system calls. */\n> while (writtenBytes < data.size()) {\n> - ssize_t ret = ::write(fd_, data.data() + writtenBytes,\n> + ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,\n> data.size() - writtenBytes);\n> if (ret <= 0)\n> break;\n> @@ -409,7 +408,7 @@ Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> if (flags & MapFlag::Private)\n> prot |= PROT_WRITE;\n>\n> - void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> + void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);\n> if (map == MAP_FAILED) {\n> error_ = -errno;\n> return {};\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 BFB71BF415\n\tfor ;\n\tMon, 29 Nov 2021 13:54:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE027605A4;\n\tMon, 29 Nov 2021 14:54:27 +0100 (CET)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11F9760592\n\tfor ;\n\tMon, 29 Nov 2021 14:54:26 +0100 (CET)","by mail-ed1-x52f.google.com with SMTP id v1so72188270edx.2\n\tfor ;\n\tMon, 29 Nov 2021 05:54:26 -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=\"JhDYCsBc\"; 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=hD0IkKtbprbiaVgyR24sQoKXlDengCGf1Az4NEf2Jds=;\n\tb=JhDYCsBcFNS6n4gskasB3hzSkuItUkONaeXsl5tzNf0IaCSzU4emcY1YZHnp3yJgEd\n\tfhU/M4aWM8hRFLzB5WCCdQPCLCg8bcaxEDYZw5d1exSKi6v0XvZi/G/iOyjze4aG8fm3\n\tUQiJcRp15ldmFLV+vUQad5PJzTkvr2zBDnWh8=","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=hD0IkKtbprbiaVgyR24sQoKXlDengCGf1Az4NEf2Jds=;\n\tb=TXc0YIIjs1eL03QmS680lQvn8np7RJZVOdkBtx/5rnp/fMRZwBw0WiHQwIQNxsCSv6\n\tkUo9qh1uxFk2eTQabUIQxaKfaDCyP6gsy3LZOjpDn0TsnJRdGjiNTUt+U3fkQzsmdd0X\n\tKmJvgfjDH3MkoOm/2ipzmdvS7fpdfUco0GunxGhfr6IMsb+RKdt09qko/oEt9FFMaY1/\n\t1E859wIRNLHlXZRjl6H+DZIiF8gamvbKQkqlgM6o9F7UlcyxjBvPBZC3c2ehoDmywmiV\n\tMwyeypTT8MU0Aj9uUrkD793cOgrBi1Upyyy5rWzvJhT0ct5EZ7MHTfSRVK3OuB4P5sRv\n\tH1eA==","X-Gm-Message-State":"AOAM530LtoGhfomNJgop6csE/Mdomueb187cFY2soUQP8fdASJoYG6UH\n\trW/sFuJHLMiG9P1QHtLPNGKvH2z03oh3Fzn9WmpGTEaZY58=","X-Google-Smtp-Source":"ABdhPJyfMCxthS3xLZGJQzqOnOXyFfajJLBruFpi1DOFR7lLM1VVR+KhLjBSaVfj1NN/2qLmSuLV4z5Mhp7nmhHjhhY=","X-Received":"by 2002:aa7:c406:: with SMTP id j6mr76087305edq.76.1638194063844;\n\tMon, 29 Nov 2021 05:54:23 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-10-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211128235752.10836-10-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Mon, 29 Nov 2021 22:54:12 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","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":21338,"web_url":"https://patchwork.libcamera.org/comment/21338/","msgid":"<20211129152221.pk7whfcp4rkpveku@uno.localdomain>","date":"2021-11-29T15:22:21","subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","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:44AM +0200, Laurent Pinchart wrote:\n> From: Hirokazu Honda \n>\n> Manages the file descriptor owned by File by UniqueFD.\n>\n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Laurent Pinchart \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> include/libcamera/base/file.h | 5 +++--\n> src/libcamera/base/file.cpp | 25 ++++++++++++-------------\n> 2 files changed, 15 insertions(+), 15 deletions(-)\n>\n> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> index 996751a7ab72..47769da7abc2 100644\n> --- a/include/libcamera/base/file.h\n> +++ b/include/libcamera/base/file.h\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n>\n> namespace libcamera {\n>\n> @@ -50,7 +51,7 @@ public:\n> \tbool exists() const;\n>\n> \tbool open(OpenMode mode);\n> -\tbool isOpen() const { return fd_ != -1; }\n> +\tbool isOpen() const { return fd_.isValid(); }\n> \tOpenMode openMode() const { return mode_; }\n> \tvoid close();\n>\n> @@ -76,7 +77,7 @@ private:\n> \tvoid unmapAll();\n>\n> \tstd::string name_;\n> -\tint fd_;\n> +\tUniqueFD fd_;\n> \tOpenMode mode_;\n>\n> \tint error_;\n> diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> index 7043f9461cf7..66c73c406198 100644\n> --- a/src/libcamera/base/file.cpp\n> +++ b/src/libcamera/base/file.cpp\n> @@ -84,7 +84,7 @@ LOG_DEFINE_CATEGORY(File)\n> * before performing I/O operations.\n> */\n> File::File(const std::string &name)\n> -\t: name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> +\t: name_(name), mode_(OpenModeFlag::NotOpen), error_(0)\n> {\n> }\n>\n> @@ -95,7 +95,7 @@ File::File(const std::string &name)\n> * setFileName().\n> */\n> File::File()\n> -\t: fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> +\t: mode_(OpenModeFlag::NotOpen), error_(0)\n> {\n> }\n>\n> @@ -178,8 +178,8 @@ bool File::open(File::OpenMode mode)\n> \tif (mode & OpenModeFlag::WriteOnly)\n> \t\tflags |= O_CREAT;\n>\n> -\tfd_ = ::open(name_.c_str(), flags, 0666);\n> -\tif (fd_ < 0) {\n> +\tfd_ = UniqueFD(::open(name_.c_str(), flags, 0666));\n> +\tif (!fd_.isValid()) {\n> \t\terror_ = -errno;\n> \t\treturn false;\n> \t}\n> @@ -210,11 +210,10 @@ bool File::open(File::OpenMode mode)\n> */\n> void File::close()\n> {\n> -\tif (fd_ == -1)\n> +\tif (!fd_.isValid())\n> \t\treturn;\n>\n> -\t::close(fd_);\n> -\tfd_ = -1;\n> +\tfd_.reset();\n> \tmode_ = OpenModeFlag::NotOpen;\n> }\n>\n> @@ -244,7 +243,7 @@ ssize_t File::size() const\n> \t\treturn -EINVAL;\n>\n> \tstruct stat st;\n> -\tint ret = fstat(fd_, &st);\n> +\tint ret = fstat(fd_.get(), &st);\n> \tif (ret < 0)\n> \t\treturn -errno;\n>\n> @@ -263,7 +262,7 @@ off_t File::pos() const\n> \tif (!isOpen())\n> \t\treturn 0;\n>\n> -\treturn lseek(fd_, 0, SEEK_CUR);\n> +\treturn lseek(fd_.get(), 0, SEEK_CUR);\n> }\n>\n> /**\n> @@ -277,7 +276,7 @@ off_t File::seek(off_t pos)\n> \tif (!isOpen())\n> \t\treturn -EINVAL;\n>\n> -\toff_t ret = lseek(fd_, pos, SEEK_SET);\n> +\toff_t ret = lseek(fd_.get(), pos, SEEK_SET);\n> \tif (ret < 0)\n> \t\treturn -errno;\n>\n> @@ -309,7 +308,7 @@ ssize_t File::read(const Span &data)\n>\n> \t/* Retry in case of interrupted system calls. */\n> \twhile (readBytes < data.size()) {\n> -\t\tret = ::read(fd_, data.data() + readBytes,\n> +\t\tret = ::read(fd_.get(), data.data() + readBytes,\n> \t\t\t data.size() - readBytes);\n> \t\tif (ret <= 0)\n> \t\t\tbreak;\n> @@ -346,7 +345,7 @@ ssize_t File::write(const Span &data)\n>\n> \t/* Retry in case of interrupted system calls. */\n> \twhile (writtenBytes < data.size()) {\n> -\t\tssize_t ret = ::write(fd_, data.data() + writtenBytes,\n> +\t\tssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,\n> \t\t\t\t data.size() - writtenBytes);\n> \t\tif (ret <= 0)\n> \t\t\tbreak;\n> @@ -409,7 +408,7 @@ Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> \tif (flags & MapFlag::Private)\n> \t\tprot |= PROT_WRITE;\n>\n> -\tvoid *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> +\tvoid *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);\n> \tif (map == MAP_FAILED) {\n> \t\terror_ = -errno;\n> \t\treturn {};\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 1B8D2BF415\n\tfor ;\n\tMon, 29 Nov 2021 15:21:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4AD01605A5;\n\tMon, 29 Nov 2021 16:21:30 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 278696059E\n\tfor ;\n\tMon, 29 Nov 2021 16:21:29 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 7D246100007;\n\tMon, 29 Nov 2021 15:21:28 +0000 (UTC)"],"Date":"Mon, 29 Nov 2021 16:22:21 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211129152221.pk7whfcp4rkpveku@uno.localdomain>","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211128235752.10836-10-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","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":21356,"web_url":"https://patchwork.libcamera.org/comment/21356/","msgid":"","date":"2021-11-29T16:52:31","subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Nov 29, 2021 at 10:54:12PM +0900, Hirokazu Honda wrote:\n> On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote:\n> >\n> > From: Hirokazu Honda \n> >\n> > Manages the file descriptor owned by File by UniqueFD.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/file.h | 5 +++--\n> > src/libcamera/base/file.cpp | 25 ++++++++++++-------------\n> > 2 files changed, 15 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > index 996751a7ab72..47769da7abc2 100644\n> > --- a/include/libcamera/base/file.h\n> > +++ b/include/libcamera/base/file.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> >\n> > namespace libcamera {\n> >\n> > @@ -50,7 +51,7 @@ public:\n> > bool exists() const;\n> >\n> > bool open(OpenMode mode);\n> > - bool isOpen() const { return fd_ != -1; }\n> > + bool isOpen() const { return fd_.isValid(); }\n> > OpenMode openMode() const { return mode_; }\n> > void close();\n> >\n> > @@ -76,7 +77,7 @@ private:\n> > void unmapAll();\n> >\n> > std::string name_;\n> > - int fd_;\n> > + UniqueFD fd_;\n> > OpenMode mode_;\n> >\n> > int error_;\n> > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> > index 7043f9461cf7..66c73c406198 100644\n> > --- a/src/libcamera/base/file.cpp\n> > +++ b/src/libcamera/base/file.cpp\n> > @@ -84,7 +84,7 @@ LOG_DEFINE_CATEGORY(File)\n> > * before performing I/O operations.\n> > */\n> > File::File(const std::string &name)\n> > - : name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > + : name_(name), mode_(OpenModeFlag::NotOpen), error_(0)\n> > {\n> > }\n> >\n> > @@ -95,7 +95,7 @@ File::File(const std::string &name)\n> > * setFileName().\n> > */\n> > File::File()\n> > - : fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > + : mode_(OpenModeFlag::NotOpen), error_(0)\n> > {\n> > }\n> >\n> > @@ -178,8 +178,8 @@ bool File::open(File::OpenMode mode)\n> > if (mode & OpenModeFlag::WriteOnly)\n> > flags |= O_CREAT;\n> >\n> > - fd_ = ::open(name_.c_str(), flags, 0666);\n> > - if (fd_ < 0) {\n> > + fd_ = UniqueFD(::open(name_.c_str(), flags, 0666));\n> \n> This is not quite related to this patch series.\n> Chrome uses HANDLE_EINTR macro [1] to retry system call in the case of EINTR.\n> It is often used with dup and open call. Shall we use it in libcamera?\n\nThat's a good question. If we want to handle syscall retries (isn't the\nlibc support to do that by the way ?), I think we should use the File\nclass (or a new similar class) and extend it with ioctl support, instead\nof adding manual retries everywhere.\n\n> [1] https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=3553913abdd97123c3937277f26cba44e6eacacf\n> \n> > + if (!fd_.isValid()) {\n> > error_ = -errno;\n> > return false;\n> > }\n> > @@ -210,11 +210,10 @@ bool File::open(File::OpenMode mode)\n> > */\n> > void File::close()\n> > {\n> > - if (fd_ == -1)\n> > + if (!fd_.isValid())\n> > return;\n> >\n> > - ::close(fd_);\n> > - fd_ = -1;\n> > + fd_.reset();\n> > mode_ = OpenModeFlag::NotOpen;\n> > }\n> >\n> > @@ -244,7 +243,7 @@ ssize_t File::size() const\n> > return -EINVAL;\n> >\n> > struct stat st;\n> > - int ret = fstat(fd_, &st);\n> > + int ret = fstat(fd_.get(), &st);\n> > if (ret < 0)\n> > return -errno;\n> >\n> > @@ -263,7 +262,7 @@ off_t File::pos() const\n> > if (!isOpen())\n> > return 0;\n> >\n> > - return lseek(fd_, 0, SEEK_CUR);\n> > + return lseek(fd_.get(), 0, SEEK_CUR);\n> > }\n> >\n> > /**\n> > @@ -277,7 +276,7 @@ off_t File::seek(off_t pos)\n> > if (!isOpen())\n> > return -EINVAL;\n> >\n> > - off_t ret = lseek(fd_, pos, SEEK_SET);\n> > + off_t ret = lseek(fd_.get(), pos, SEEK_SET);\n> > if (ret < 0)\n> > return -errno;\n> >\n> > @@ -309,7 +308,7 @@ ssize_t File::read(const Span &data)\n> >\n> > /* Retry in case of interrupted system calls. */\n> > while (readBytes < data.size()) {\n> > - ret = ::read(fd_, data.data() + readBytes,\n> > + ret = ::read(fd_.get(), data.data() + readBytes,\n> > data.size() - readBytes);\n> > if (ret <= 0)\n> > break;\n> > @@ -346,7 +345,7 @@ ssize_t File::write(const Span &data)\n> >\n> > /* Retry in case of interrupted system calls. */\n> > while (writtenBytes < data.size()) {\n> > - ssize_t ret = ::write(fd_, data.data() + writtenBytes,\n> > + ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,\n> > data.size() - writtenBytes);\n> > if (ret <= 0)\n> > break;\n> > @@ -409,7 +408,7 @@ Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> > if (flags & MapFlag::Private)\n> > prot |= PROT_WRITE;\n> >\n> > - void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> > + void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);\n> > if (map == MAP_FAILED) {\n> > error_ = -errno;\n> > return {};","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 C3464BF415\n\tfor ;\n\tMon, 29 Nov 2021 16:52:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFE19605A4;\n\tMon, 29 Nov 2021 17:52:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1091460592\n\tfor ;\n\tMon, 29 Nov 2021 17:52:56 +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 7AFF42A5;\n\tMon, 29 Nov 2021 17:52:55 +0100 (CET)"],"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=\"cxCgGKAz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638204775;\n\tbh=0IPsQ2WQ9XvBYts6rhgwj4UbgGvezKCNcK8R7SnWJpg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cxCgGKAzE62DzlwvZelvJUxSP25NYoW5Ie//0EeQBBD6UnNhVIIUQvlNR5qLzHfy5\n\tl0/NUfbv2sYgYlaDS2/uDg16Y8tLPiWJV20RTtYSq5Gp97O6yvUnKM42ESk0Ppdz8c\n\tG4Ofmg7V7XZbZLqYWV/C1nOb9PwGJxgwl1oXJ14c=","Date":"Mon, 29 Nov 2021 18:52:31 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-10-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] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","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":21381,"web_url":"https://patchwork.libcamera.org/comment/21381/","msgid":"","date":"2021-11-30T01:01:01","subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Nov 30, 2021 at 1:52 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Nov 29, 2021 at 10:54:12PM +0900, Hirokazu Honda wrote:\n> > On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote:\n> > >\n> > > From: Hirokazu Honda \n> > >\n> > > Manages the file descriptor owned by File by UniqueFD.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/base/file.h | 5 +++--\n> > > src/libcamera/base/file.cpp | 25 ++++++++++++-------------\n> > > 2 files changed, 15 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > > index 996751a7ab72..47769da7abc2 100644\n> > > --- a/include/libcamera/base/file.h\n> > > +++ b/include/libcamera/base/file.h\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > namespace libcamera {\n> > >\n> > > @@ -50,7 +51,7 @@ public:\n> > > bool exists() const;\n> > >\n> > > bool open(OpenMode mode);\n> > > - bool isOpen() const { return fd_ != -1; }\n> > > + bool isOpen() const { return fd_.isValid(); }\n> > > OpenMode openMode() const { return mode_; }\n> > > void close();\n> > >\n> > > @@ -76,7 +77,7 @@ private:\n> > > void unmapAll();\n> > >\n> > > std::string name_;\n> > > - int fd_;\n> > > + UniqueFD fd_;\n> > > OpenMode mode_;\n> > >\n> > > int error_;\n> > > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> > > index 7043f9461cf7..66c73c406198 100644\n> > > --- a/src/libcamera/base/file.cpp\n> > > +++ b/src/libcamera/base/file.cpp\n> > > @@ -84,7 +84,7 @@ LOG_DEFINE_CATEGORY(File)\n> > > * before performing I/O operations.\n> > > */\n> > > File::File(const std::string &name)\n> > > - : name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > + : name_(name), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > {\n> > > }\n> > >\n> > > @@ -95,7 +95,7 @@ File::File(const std::string &name)\n> > > * setFileName().\n> > > */\n> > > File::File()\n> > > - : fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > + : mode_(OpenModeFlag::NotOpen), error_(0)\n> > > {\n> > > }\n> > >\n> > > @@ -178,8 +178,8 @@ bool File::open(File::OpenMode mode)\n> > > if (mode & OpenModeFlag::WriteOnly)\n> > > flags |= O_CREAT;\n> > >\n> > > - fd_ = ::open(name_.c_str(), flags, 0666);\n> > > - if (fd_ < 0) {\n> > > + fd_ = UniqueFD(::open(name_.c_str(), flags, 0666));\n> >\n> > This is not quite related to this patch series.\n> > Chrome uses HANDLE_EINTR macro [1] to retry system call in the case of EINTR.\n> > It is often used with dup and open call. Shall we use it in libcamera?\n>\n> That's a good question. If we want to handle syscall retries (isn't the\n> libc support to do that by the way ?), I think we should use the File\n> class (or a new similar class) and extend it with ioctl support, instead\n> of adding manual retries everywhere.\n>\n\nIMO, just like UniqueFD(HANDLE_EINTR(dup(fd))) is not so harmful,\ncompared to UniqueFD(dup(d)).\n\n-Hiro\n> > [1] https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=3553913abdd97123c3937277f26cba44e6eacacf\n> >\n> > > + if (!fd_.isValid()) {\n> > > error_ = -errno;\n> > > return false;\n> > > }\n> > > @@ -210,11 +210,10 @@ bool File::open(File::OpenMode mode)\n> > > */\n> > > void File::close()\n> > > {\n> > > - if (fd_ == -1)\n> > > + if (!fd_.isValid())\n> > > return;\n> > >\n> > > - ::close(fd_);\n> > > - fd_ = -1;\n> > > + fd_.reset();\n> > > mode_ = OpenModeFlag::NotOpen;\n> > > }\n> > >\n> > > @@ -244,7 +243,7 @@ ssize_t File::size() const\n> > > return -EINVAL;\n> > >\n> > > struct stat st;\n> > > - int ret = fstat(fd_, &st);\n> > > + int ret = fstat(fd_.get(), &st);\n> > > if (ret < 0)\n> > > return -errno;\n> > >\n> > > @@ -263,7 +262,7 @@ off_t File::pos() const\n> > > if (!isOpen())\n> > > return 0;\n> > >\n> > > - return lseek(fd_, 0, SEEK_CUR);\n> > > + return lseek(fd_.get(), 0, SEEK_CUR);\n> > > }\n> > >\n> > > /**\n> > > @@ -277,7 +276,7 @@ off_t File::seek(off_t pos)\n> > > if (!isOpen())\n> > > return -EINVAL;\n> > >\n> > > - off_t ret = lseek(fd_, pos, SEEK_SET);\n> > > + off_t ret = lseek(fd_.get(), pos, SEEK_SET);\n> > > if (ret < 0)\n> > > return -errno;\n> > >\n> > > @@ -309,7 +308,7 @@ ssize_t File::read(const Span &data)\n> > >\n> > > /* Retry in case of interrupted system calls. */\n> > > while (readBytes < data.size()) {\n> > > - ret = ::read(fd_, data.data() + readBytes,\n> > > + ret = ::read(fd_.get(), data.data() + readBytes,\n> > > data.size() - readBytes);\n> > > if (ret <= 0)\n> > > break;\n> > > @@ -346,7 +345,7 @@ ssize_t File::write(const Span &data)\n> > >\n> > > /* Retry in case of interrupted system calls. */\n> > > while (writtenBytes < data.size()) {\n> > > - ssize_t ret = ::write(fd_, data.data() + writtenBytes,\n> > > + ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,\n> > > data.size() - writtenBytes);\n> > > if (ret <= 0)\n> > > break;\n> > > @@ -409,7 +408,7 @@ Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> > > if (flags & MapFlag::Private)\n> > > prot |= PROT_WRITE;\n> > >\n> > > - void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> > > + void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);\n> > > if (map == MAP_FAILED) {\n> > > error_ = -errno;\n> > > return {};\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 AAB99BDB13\n\tfor ;\n\tTue, 30 Nov 2021 01:01:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CECB4605A6;\n\tTue, 30 Nov 2021 02:01:14 +0100 (CET)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78E31604FC\n\tfor ;\n\tTue, 30 Nov 2021 02:01:13 +0100 (CET)","by mail-ed1-x530.google.com with SMTP id o20so79118746eds.10\n\tfor ;\n\tMon, 29 Nov 2021 17:01:13 -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=\"ZK6rseVT\"; 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=/o6wu1PoDkTxxrmO6Bp4FTEyXJ5ubnBh4AliPGz33PI=;\n\tb=ZK6rseVTU7KoN7SwWtl9pQyMYyYwiuFyXQnd3lFnuLicj7qd+TyRLEehtsLMdvx5J2\n\trbkyLdsrSsRh6VCjCNoJWoo2LQS8N6adNtx7SLG1e8juIMTCrHUdtFyR2smX8/G6lB8H\n\tZVIckk9WfF87wUa/IV/KBLS0tK+K9S2XoStLE=","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=/o6wu1PoDkTxxrmO6Bp4FTEyXJ5ubnBh4AliPGz33PI=;\n\tb=TkP3PBVVkeD5Ht9JXIYcIks4BH+C9Nh7cdFAcSHloMtkc9ZAQ/Qg/NrozrMpdIil0k\n\tvp7cNAXy52V09+mcg/DPuWLETkeaoYean5nUIjUxrrxfp7dXF+aDBA314gU3/rPmaDW8\n\tk6aDWuBnz+kQPVKI4nCR6gCNRgCZ/VRaqPFslG+s5YmaFk7Pl3gj6KZRHofCHsJQVAIv\n\tgpzaOur51nBCIrdwIl6uVMFmvpEr+kBKG0NQB8MwOnnWA0I4737V0692DBbghCS1D+GW\n\t7O+ulgxXw6SG9f1voNKICr+fE30ozhDiSEqZS6ph82G34djOVfs9CqXTJSa4qUcH1BhE\n\tNkPA==","X-Gm-Message-State":"AOAM5315TDaDYnQuEN+zTdu0Fq3+0TYsyngnneEOt4ZBcr7JLktv0Ix7\n\tbw+11NznQxqtC/zkxi/a/fm+DQGMkZKDDxEfFq71r59GNZc=","X-Google-Smtp-Source":"ABdhPJysNLFkh8McsbrbetmpEYlqac4hBxJsOWxRkoSw7o3STMKImU/z3qXcwwXthwHud3gft92QLfQj0Nis3wVMwQw=","X-Received":"by 2002:aa7:c406:: with SMTP id j6mr80624755edq.76.1638234072887;\n\tMon, 29 Nov 2021 17:01:12 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-10-laurent.pinchart@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 30 Nov 2021 10:01:01 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","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":21384,"web_url":"https://patchwork.libcamera.org/comment/21384/","msgid":"","date":"2021-11-30T01:48:26","subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Nov 30, 2021 at 10:01:01AM +0900, Hirokazu Honda wrote:\n> On Tue, Nov 30, 2021 at 1:52 AM Laurent Pinchart wrote:\n> > On Mon, Nov 29, 2021 at 10:54:12PM +0900, Hirokazu Honda wrote:\n> > > On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote:\n> > > >\n> > > > From: Hirokazu Honda \n> > > >\n> > > > Manages the file descriptor owned by File by UniqueFD.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Laurent Pinchart \n> > > > ---\n> > > > include/libcamera/base/file.h | 5 +++--\n> > > > src/libcamera/base/file.cpp | 25 ++++++++++++-------------\n> > > > 2 files changed, 15 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > > > index 996751a7ab72..47769da7abc2 100644\n> > > > --- a/include/libcamera/base/file.h\n> > > > +++ b/include/libcamera/base/file.h\n> > > > @@ -17,6 +17,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > @@ -50,7 +51,7 @@ public:\n> > > > bool exists() const;\n> > > >\n> > > > bool open(OpenMode mode);\n> > > > - bool isOpen() const { return fd_ != -1; }\n> > > > + bool isOpen() const { return fd_.isValid(); }\n> > > > OpenMode openMode() const { return mode_; }\n> > > > void close();\n> > > >\n> > > > @@ -76,7 +77,7 @@ private:\n> > > > void unmapAll();\n> > > >\n> > > > std::string name_;\n> > > > - int fd_;\n> > > > + UniqueFD fd_;\n> > > > OpenMode mode_;\n> > > >\n> > > > int error_;\n> > > > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> > > > index 7043f9461cf7..66c73c406198 100644\n> > > > --- a/src/libcamera/base/file.cpp\n> > > > +++ b/src/libcamera/base/file.cpp\n> > > > @@ -84,7 +84,7 @@ LOG_DEFINE_CATEGORY(File)\n> > > > * before performing I/O operations.\n> > > > */\n> > > > File::File(const std::string &name)\n> > > > - : name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > + : name_(name), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -95,7 +95,7 @@ File::File(const std::string &name)\n> > > > * setFileName().\n> > > > */\n> > > > File::File()\n> > > > - : fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > + : mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -178,8 +178,8 @@ bool File::open(File::OpenMode mode)\n> > > > if (mode & OpenModeFlag::WriteOnly)\n> > > > flags |= O_CREAT;\n> > > >\n> > > > - fd_ = ::open(name_.c_str(), flags, 0666);\n> > > > - if (fd_ < 0) {\n> > > > + fd_ = UniqueFD(::open(name_.c_str(), flags, 0666));\n> > >\n> > > This is not quite related to this patch series.\n> > > Chrome uses HANDLE_EINTR macro [1] to retry system call in the case of EINTR.\n> > > It is often used with dup and open call. Shall we use it in libcamera?\n> >\n> > That's a good question. If we want to handle syscall retries (isn't the\n> > libc support to do that by the way ?), I think we should use the File\n> > class (or a new similar class) and extend it with ioctl support, instead\n> > of adding manual retries everywhere.\n> \n> IMO, just like UniqueFD(HANDLE_EINTR(dup(fd))) is not so harmful,\n> compared to UniqueFD(dup(d)).\n\nAdding HANDLE_EINTR() manually everywhere is the best way to make sure\nit will be forgotten somewhere. I don't like that.\n\n> > > [1] https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=3553913abdd97123c3937277f26cba44e6eacacf\n> > >\n> > > > + if (!fd_.isValid()) {\n> > > > error_ = -errno;\n> > > > return false;\n> > > > }\n> > > > @@ -210,11 +210,10 @@ bool File::open(File::OpenMode mode)\n> > > > */\n> > > > void File::close()\n> > > > {\n> > > > - if (fd_ == -1)\n> > > > + if (!fd_.isValid())\n> > > > return;\n> > > >\n> > > > - ::close(fd_);\n> > > > - fd_ = -1;\n> > > > + fd_.reset();\n> > > > mode_ = OpenModeFlag::NotOpen;\n> > > > }\n> > > >\n> > > > @@ -244,7 +243,7 @@ ssize_t File::size() const\n> > > > return -EINVAL;\n> > > >\n> > > > struct stat st;\n> > > > - int ret = fstat(fd_, &st);\n> > > > + int ret = fstat(fd_.get(), &st);\n> > > > if (ret < 0)\n> > > > return -errno;\n> > > >\n> > > > @@ -263,7 +262,7 @@ off_t File::pos() const\n> > > > if (!isOpen())\n> > > > return 0;\n> > > >\n> > > > - return lseek(fd_, 0, SEEK_CUR);\n> > > > + return lseek(fd_.get(), 0, SEEK_CUR);\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -277,7 +276,7 @@ off_t File::seek(off_t pos)\n> > > > if (!isOpen())\n> > > > return -EINVAL;\n> > > >\n> > > > - off_t ret = lseek(fd_, pos, SEEK_SET);\n> > > > + off_t ret = lseek(fd_.get(), pos, SEEK_SET);\n> > > > if (ret < 0)\n> > > > return -errno;\n> > > >\n> > > > @@ -309,7 +308,7 @@ ssize_t File::read(const Span &data)\n> > > >\n> > > > /* Retry in case of interrupted system calls. */\n> > > > while (readBytes < data.size()) {\n> > > > - ret = ::read(fd_, data.data() + readBytes,\n> > > > + ret = ::read(fd_.get(), data.data() + readBytes,\n> > > > data.size() - readBytes);\n> > > > if (ret <= 0)\n> > > > break;\n> > > > @@ -346,7 +345,7 @@ ssize_t File::write(const Span &data)\n> > > >\n> > > > /* Retry in case of interrupted system calls. */\n> > > > while (writtenBytes < data.size()) {\n> > > > - ssize_t ret = ::write(fd_, data.data() + writtenBytes,\n> > > > + ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,\n> > > > data.size() - writtenBytes);\n> > > > if (ret <= 0)\n> > > > break;\n> > > > @@ -409,7 +408,7 @@ Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> > > > if (flags & MapFlag::Private)\n> > > > prot |= PROT_WRITE;\n> > > >\n> > > > - void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> > > > + void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);\n> > > > if (map == MAP_FAILED) {\n> > > > error_ = -errno;\n> > > > return {};","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 78C09BDB13\n\tfor ;\n\tTue, 30 Nov 2021 01:48:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0AB1605A4;\n\tTue, 30 Nov 2021 02:48:52 +0100 (CET)","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 0CC27604FC\n\tfor ;\n\tTue, 30 Nov 2021 02:48:51 +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 75C122A5;\n\tTue, 30 Nov 2021 02:48:50 +0100 (CET)"],"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=\"mUt1JCfJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638236930;\n\tbh=qB7bfrh0oy6sP6f8Wfco1p+B5jj9j45hPG4bsIaAEGE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mUt1JCfJeFahsEDMYaQDxzWihhH2KRcPJ98uUie5JJEpJrVFFIY8OOjyD8wItYOWI\n\tTccAiPivVNAFjQ+4CA7ext8DKXMpp82cJoGrgdIq2DuGlkVFxA+EEAEx7oI7hSqP6z\n\t7HroEjHxwSmkLB0bmopgpN/tb01OBp/L7wsLFR+0=","Date":"Tue, 30 Nov 2021 03:48:26 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-10-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] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","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":21385,"web_url":"https://patchwork.libcamera.org/comment/21385/","msgid":"","date":"2021-11-30T02:04:48","subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Nov 30, 2021 at 10:48 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Nov 30, 2021 at 10:01:01AM +0900, Hirokazu Honda wrote:\n> > On Tue, Nov 30, 2021 at 1:52 AM Laurent Pinchart wrote:\n> > > On Mon, Nov 29, 2021 at 10:54:12PM +0900, Hirokazu Honda wrote:\n> > > > On Mon, Nov 29, 2021 at 8:58 AM Laurent Pinchart wrote:\n> > > > >\n> > > > > From: Hirokazu Honda \n> > > > >\n> > > > > Manages the file descriptor owned by File by UniqueFD.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > ---\n> > > > > include/libcamera/base/file.h | 5 +++--\n> > > > > src/libcamera/base/file.cpp | 25 ++++++++++++-------------\n> > > > > 2 files changed, 15 insertions(+), 15 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> > > > > index 996751a7ab72..47769da7abc2 100644\n> > > > > --- a/include/libcamera/base/file.h\n> > > > > +++ b/include/libcamera/base/file.h\n> > > > > @@ -17,6 +17,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > >\n> > > > > namespace libcamera {\n> > > > >\n> > > > > @@ -50,7 +51,7 @@ public:\n> > > > > bool exists() const;\n> > > > >\n> > > > > bool open(OpenMode mode);\n> > > > > - bool isOpen() const { return fd_ != -1; }\n> > > > > + bool isOpen() const { return fd_.isValid(); }\n> > > > > OpenMode openMode() const { return mode_; }\n> > > > > void close();\n> > > > >\n> > > > > @@ -76,7 +77,7 @@ private:\n> > > > > void unmapAll();\n> > > > >\n> > > > > std::string name_;\n> > > > > - int fd_;\n> > > > > + UniqueFD fd_;\n> > > > > OpenMode mode_;\n> > > > >\n> > > > > int error_;\n> > > > > diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> > > > > index 7043f9461cf7..66c73c406198 100644\n> > > > > --- a/src/libcamera/base/file.cpp\n> > > > > +++ b/src/libcamera/base/file.cpp\n> > > > > @@ -84,7 +84,7 @@ LOG_DEFINE_CATEGORY(File)\n> > > > > * before performing I/O operations.\n> > > > > */\n> > > > > File::File(const std::string &name)\n> > > > > - : name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > > + : name_(name), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > @@ -95,7 +95,7 @@ File::File(const std::string &name)\n> > > > > * setFileName().\n> > > > > */\n> > > > > File::File()\n> > > > > - : fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > > + : mode_(OpenModeFlag::NotOpen), error_(0)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > @@ -178,8 +178,8 @@ bool File::open(File::OpenMode mode)\n> > > > > if (mode & OpenModeFlag::WriteOnly)\n> > > > > flags |= O_CREAT;\n> > > > >\n> > > > > - fd_ = ::open(name_.c_str(), flags, 0666);\n> > > > > - if (fd_ < 0) {\n> > > > > + fd_ = UniqueFD(::open(name_.c_str(), flags, 0666));\n> > > >\n> > > > This is not quite related to this patch series.\n> > > > Chrome uses HANDLE_EINTR macro [1] to retry system call in the case of EINTR.\n> > > > It is often used with dup and open call. Shall we use it in libcamera?\n> > >\n> > > That's a good question. If we want to handle syscall retries (isn't the\n> > > libc support to do that by the way ?), I think we should use the File\n> > > class (or a new similar class) and extend it with ioctl support, instead\n> > > of adding manual retries everywhere.\n> >\n> > IMO, just like UniqueFD(HANDLE_EINTR(dup(fd))) is not so harmful,\n> > compared to UniqueFD(dup(d)).\n>\n> Adding HANDLE_EINTR() manually everywhere is the best way to make sure\n> it will be forgotten somewhere. I don't like that.\n>\n\nAck.\n\n> > > > [1] https://source.chromium.org/chromium/chromium/src/+/main:base/posix/eintr_wrapper.h;l=28;drc=3553913abdd97123c3937277f26cba44e6eacacf\n> > > >\n> > > > > + if (!fd_.isValid()) {\n> > > > > error_ = -errno;\n> > > > > return false;\n> > > > > }\n> > > > > @@ -210,11 +210,10 @@ bool File::open(File::OpenMode mode)\n> > > > > */\n> > > > > void File::close()\n> > > > > {\n> > > > > - if (fd_ == -1)\n> > > > > + if (!fd_.isValid())\n> > > > > return;\n> > > > >\n> > > > > - ::close(fd_);\n> > > > > - fd_ = -1;\n> > > > > + fd_.reset();\n> > > > > mode_ = OpenModeFlag::NotOpen;\n> > > > > }\n> > > > >\n> > > > > @@ -244,7 +243,7 @@ ssize_t File::size() const\n> > > > > return -EINVAL;\n> > > > >\n> > > > > struct stat st;\n> > > > > - int ret = fstat(fd_, &st);\n> > > > > + int ret = fstat(fd_.get(), &st);\n> > > > > if (ret < 0)\n> > > > > return -errno;\n> > > > >\n> > > > > @@ -263,7 +262,7 @@ off_t File::pos() const\n> > > > > if (!isOpen())\n> > > > > return 0;\n> > > > >\n> > > > > - return lseek(fd_, 0, SEEK_CUR);\n> > > > > + return lseek(fd_.get(), 0, SEEK_CUR);\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -277,7 +276,7 @@ off_t File::seek(off_t pos)\n> > > > > if (!isOpen())\n> > > > > return -EINVAL;\n> > > > >\n> > > > > - off_t ret = lseek(fd_, pos, SEEK_SET);\n> > > > > + off_t ret = lseek(fd_.get(), pos, SEEK_SET);\n> > > > > if (ret < 0)\n> > > > > return -errno;\n> > > > >\n> > > > > @@ -309,7 +308,7 @@ ssize_t File::read(const Span &data)\n> > > > >\n> > > > > /* Retry in case of interrupted system calls. */\n> > > > > while (readBytes < data.size()) {\n> > > > > - ret = ::read(fd_, data.data() + readBytes,\n> > > > > + ret = ::read(fd_.get(), data.data() + readBytes,\n> > > > > data.size() - readBytes);\n> > > > > if (ret <= 0)\n> > > > > break;\n> > > > > @@ -346,7 +345,7 @@ ssize_t File::write(const Span &data)\n> > > > >\n> > > > > /* Retry in case of interrupted system calls. */\n> > > > > while (writtenBytes < data.size()) {\n> > > > > - ssize_t ret = ::write(fd_, data.data() + writtenBytes,\n> > > > > + ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,\n> > > > > data.size() - writtenBytes);\n> > > > > if (ret <= 0)\n> > > > > break;\n> > > > > @@ -409,7 +408,7 @@ Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> > > > > if (flags & MapFlag::Private)\n> > > > > prot |= PROT_WRITE;\n> > > > >\n> > > > > - void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> > > > > + void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);\n> > > > > if (map == MAP_FAILED) {\n> > > > > error_ = -errno;\n> > > > > return {};\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 5E94DBF415\n\tfor ;\n\tTue, 30 Nov 2021 02:05:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72C18605B4;\n\tTue, 30 Nov 2021 03:05:01 +0100 (CET)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7F30604FC\n\tfor ;\n\tTue, 30 Nov 2021 03:04:59 +0100 (CET)","by mail-ed1-x52a.google.com with SMTP id t5so80330546edd.0\n\tfor ;\n\tMon, 29 Nov 2021 18:04:59 -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=\"WgbTeBN8\"; 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=BeUh91Gw9tfNOuJAR8qty14DSgY09jbD/CNWhM2mQY8=;\n\tb=WgbTeBN8in3OMjPYyCx8OdPbhcIHQxzP62ykaPecLS00116d6EmCU0bUrJRqVuNOnf\n\t6haYGHPDR+AwgH/BS446dZIfiQ88qNq9CCbYAwkj4U8CAaxhLPZnrpIgT5npd3zTN0on\n\tZbs01lBLuBuQ6KKP7kh58q/TRCEG/+4op6Rrc=","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=BeUh91Gw9tfNOuJAR8qty14DSgY09jbD/CNWhM2mQY8=;\n\tb=DGTvdIvsEcyV35dVJgdG0JZCpeMLZV4pHoaTF1lgU+M5tVz0yXD5D6WL3JFbC1aynV\n\teBWL+a3zLHXO3cDmytE8/ZQ5yqTSZS0ehQ1JMX4ggYJmAOtc65oMvKXuyv4rxU0nPh1J\n\tsZpVLvxWqlDhhL92LKgubvEFNbmfce181NuyCRC9+YSq12TQ7aIdf/scB42QfQHIg18V\n\t40EQip6VzEK4sNiIiVpvi067FSucv0MbyljRdC3VHy1t+khEaLp1pf3DetloiQjtE5Yc\n\tr5J3CqyyY18pN0ix/6y8PMdc+Ljy5irRTKnVCbV9+LDm0n/ToqgxRG2s5mN1XwTwfgSn\n\t2CoA==","X-Gm-Message-State":"AOAM533DuZQzgm3nNqqUmsuoVSFfbUhbcGae4Mb1uOtRk4hb+DzPqR3Y\n\t85fuDG0Rni+Nd2TsZE1dGJcSCsfsPEnyd4zN3aEMiXFY5BU=","X-Google-Smtp-Source":"ABdhPJyGKFIG7UknuwBvrww+6POtd+Zmt5F5dJoUII4Yv7YXUcJ+/ED0OGfNpLd9ufc6zCTWZiQHqpC/9jicIYTqTWk=","X-Received":"by 2002:a17:907:1c82:: with SMTP id\n\tnb2mr62948839ejc.218.1638237899248; \n\tMon, 29 Nov 2021 18:04:59 -0800 (PST)","MIME-Version":"1.0","References":"<20211128235752.10836-1-laurent.pinchart@ideasonboard.com>\n\t<20211128235752.10836-10-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 30 Nov 2021 11:04:48 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 09/17] libcamera: file: Manage fd\n\tby UniqueFD","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\" "}}]