[{"id":18349,"web_url":"https://patchwork.libcamera.org/comment/18349/","msgid":"<20210726095512.cfsabjhjmad43jak@uno.localdomain>","date":"2021-07-26T09:55:12","subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: file: Turn MapFlag\n\tand OpenModeFlag into enum class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sun, Jul 25, 2021 at 08:18:27PM +0300, Laurent Pinchart wrote:\n> Add type safety by turning the MapFlag and OpenModeFlag enum into enum\n> class.\n\nAh\n\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/file.h | 13 ++++++++-----\n> src/ipa/vimc/vimc.cpp | 2 +-\n> src/libcamera/base/file.cpp | 34 +++++++++++++++++-----------------\n> src/libcamera/ipa_manager.cpp | 2 +-\n> src/libcamera/ipa_module.cpp | 6 +++---\n> test/file.cpp | 32 ++++++++++++++++----------------\n> 6 files changed, 46 insertions(+), 43 deletions(-)\n>\n> diff --git a/include/libcamera/base/file.h b/include/libcamera/base/file.h\n> index eebb24024025..60851d385587 100644\n> --- a/include/libcamera/base/file.h\n> +++ b/include/libcamera/base/file.h\n> @@ -23,14 +23,14 @@ namespace libcamera {\n> class File\n> {\n> public:\n> -\tenum MapFlag {\n> -\t\tMapNoOption = 0,\n> -\t\tMapPrivate = (1 << 0),\n> +\tenum class MapFlag {\n> +\t\tNoOption = 0,\n> +\t\tPrivate = (1 << 0),\n> \t};\n>\n> \tusing MapFlags = Flags;\n>\n> -\tenum OpenModeFlag {\n> +\tenum class OpenModeFlag {\n> \t\tNotOpen = 0,\n> \t\tReadOnly = (1 << 0),\n> \t\tWriteOnly = (1 << 1),\n> @@ -62,7 +62,7 @@ public:\n> \tssize_t write(const Span &data);\n>\n> \tSpan map(off_t offset = 0, ssize_t size = -1,\n> -\t\t\t MapFlags flags = MapNoOption);\n> +\t\t\t MapFlags flags = MapFlag::NoOption);\n> \tbool unmap(uint8_t *addr);\n>\n> \tstatic bool exists(const std::string &name);\n> @@ -80,6 +80,9 @@ private:\n> \tstd::map maps_;\n> };\n>\n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(File::MapFlag)\n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(File::OpenModeFlag)\n> +\n> } /* namespace libcamera */\n>\n> #endif /* __LIBCAMERA_BASE_FILE_H__ */\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index 2f5758539a0e..0c0ee006fdc7 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -62,7 +62,7 @@ int IPAVimc::init(const IPASettings &settings)\n> \t\t<< settings.configurationFile;\n>\n> \tFile conf(settings.configurationFile);\n> -\tif (!conf.open(File::ReadOnly)) {\n> +\tif (!conf.open(File::OpenModeFlag::ReadOnly)) {\n> \t\tLOG(IPAVimc, Error) << \"Failed to open configuration file\";\n> \t\treturn -EINVAL;\n> \t}\n> diff --git a/src/libcamera/base/file.cpp b/src/libcamera/base/file.cpp\n> index 0860457421a1..376bf2ead382 100644\n> --- a/src/libcamera/base/file.cpp\n> +++ b/src/libcamera/base/file.cpp\n> @@ -45,9 +45,9 @@ LOG_DEFINE_CATEGORY(File)\n> /**\n> * \\enum File::MapFlag\n> * \\brief Flags for the File::map() function\n> - * \\var File::MapNoOption\n> + * \\var File::MapFlag::NoOption\n> * \\brief No option (used as default value)\n> - * \\var File::MapPrivate\n> + * \\var File::MapFlag::Private\n> * \\brief The memory region is mapped as private, changes are not reflected in\n> * the file constents\n> */\n> @@ -60,13 +60,13 @@ LOG_DEFINE_CATEGORY(File)\n> /**\n> * \\enum File::OpenModeFlag\n> * \\brief Mode in which a file is opened\n> - * \\var File::NotOpen\n> + * \\var File::OpenModeFlag::NotOpen\n> * \\brief The file is not open\n> - * \\var File::ReadOnly\n> + * \\var File::OpenModeFlag::ReadOnly\n> * \\brief The file is open for reading\n> - * \\var File::WriteOnly\n> + * \\var File::OpenModeFlag::WriteOnly\n> * \\brief The file is open for writing\n> - * \\var File::ReadWrite\n> + * \\var File::OpenModeFlag::ReadWrite\n> * \\brief The file is open for reading and writing\n> */\n>\n> @@ -83,7 +83,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_(NotOpen), error_(0)\n> +\t: name_(name), fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> {\n> }\n>\n> @@ -94,7 +94,7 @@ File::File(const std::string &name)\n> * setFileName().\n> */\n> File::File()\n> -\t: fd_(-1), mode_(NotOpen), error_(0)\n> +\t: fd_(-1), mode_(OpenModeFlag::NotOpen), error_(0)\n> {\n> }\n>\n> @@ -173,8 +173,8 @@ bool File::open(File::OpenMode mode)\n> \t\treturn false;\n> \t}\n>\n> -\tint flags = static_cast(mode & ReadWrite) - 1;\n> -\tif (mode & WriteOnly)\n> +\tint flags = static_cast(mode & OpenModeFlag::ReadWrite) - 1;\n> +\tif (mode & OpenModeFlag::WriteOnly)\n> \t\tflags |= O_CREAT;\n>\n> \tfd_ = ::open(name_.c_str(), flags, 0666);\n> @@ -214,7 +214,7 @@ void File::close()\n>\n> \t::close(fd_);\n> \tfd_ = -1;\n> -\tmode_ = NotOpen;\n> +\tmode_ = OpenModeFlag::NotOpen;\n> }\n>\n> /**\n> @@ -374,8 +374,8 @@ ssize_t File::write(const Span &data)\n> * offset until the end of the file.\n> *\n> * The mapping memory protection is controlled by the file open mode, unless \\a\n> - * flags contains MapPrivate in which case the region is mapped in read/write\n> - * mode.\n> + * flags contains MapFlag::Private in which case the region is mapped in\n> + * read/write mode.\n> *\n> * The error() status is updated.\n> *\n> @@ -398,14 +398,14 @@ Span File::map(off_t offset, ssize_t size, File::MapFlags flags)\n> \t\tsize -= offset;\n> \t}\n>\n> -\tint mmapFlags = flags & MapPrivate ? MAP_PRIVATE : MAP_SHARED;\n> +\tint mmapFlags = flags & MapFlag::Private ? MAP_PRIVATE : MAP_SHARED;\n>\n> \tint prot = 0;\n> -\tif (mode_ & ReadOnly)\n> +\tif (mode_ & OpenModeFlag::ReadOnly)\n> \t\tprot |= PROT_READ;\n> -\tif (mode_ & WriteOnly)\n> +\tif (mode_ & OpenModeFlag::WriteOnly)\n> \t\tprot |= PROT_WRITE;\n> -\tif (flags & MapPrivate)\n> +\tif (flags & MapFlag::Private)\n> \t\tprot |= PROT_WRITE;\n>\n> \tvoid *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 558408969c31..771150ba0b35 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -285,7 +285,7 @@ bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const\n> \t}\n>\n> \tFile file{ ipa->path() };\n> -\tif (!file.open(File::ReadOnly))\n> +\tif (!file.open(File::OpenModeFlag::ReadOnly))\n> \t\treturn false;\n>\n> \tSpan data = file.map();\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index adfb8d407697..7c52ad8d5796 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -275,7 +275,7 @@ IPAModule::~IPAModule()\n> int IPAModule::loadIPAModuleInfo()\n> {\n> \tFile file{ libPath_ };\n> -\tif (!file.open(File::ReadOnly)) {\n> +\tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n> \t\tLOG(IPAModule, Error) << \"Failed to open IPA library: \"\n> \t\t\t\t << strerror(-file.error());\n> \t\treturn file.error();\n> @@ -317,13 +317,13 @@ int IPAModule::loadIPAModuleInfo()\n>\n> \t/* Load the signature. Failures are not fatal. */\n> \tFile sign{ libPath_ + \".sign\" };\n> -\tif (!sign.open(File::ReadOnly)) {\n> +\tif (!sign.open(File::OpenModeFlag::ReadOnly)) {\n> \t\tLOG(IPAModule, Debug)\n> \t\t\t<< \"IPA module \" << libPath_ << \" is not signed\";\n> \t\treturn 0;\n> \t}\n>\n> -\tdata = sign.map(0, -1, File::MapPrivate);\n> +\tdata = sign.map(0, -1, File::MapFlag::Private);\n> \tsignature_.resize(data.size());\n> \tmemcpy(signature_.data(), data.data(), data.size());\n>\n> diff --git a/test/file.cpp b/test/file.cpp\n> index d768e3235b8c..9ac151c944b5 100644\n> --- a/test/file.cpp\n> +++ b/test/file.cpp\n> @@ -72,7 +72,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.openMode() != File::NotOpen) {\n> +\t\tif (file.openMode() != File::OpenModeFlag::NotOpen) {\n> \t\t\tcerr << \"File has invalid open mode after construction\"\n> \t\t\t << endl;\n> \t\t\treturn TestFail;\n> @@ -83,7 +83,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.open(File::ReadWrite)) {\n> +\t\tif (file.open(File::OpenModeFlag::ReadWrite)) {\n> \t\t\tcerr << \"Opening unnamed file succeeded\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -111,7 +111,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.openMode() != File::NotOpen) {\n> +\t\tif (file.openMode() != File::OpenModeFlag::NotOpen) {\n> \t\t\tcerr << \"Invalid file has invalid open mode after construction\"\n> \t\t\t << endl;\n> \t\t\treturn TestFail;\n> @@ -122,7 +122,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.open(File::ReadWrite)) {\n> +\t\tif (file.open(File::OpenModeFlag::ReadWrite)) {\n> \t\t\tcerr << \"Opening invalid file succeeded\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -140,7 +140,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.openMode() != File::NotOpen) {\n> +\t\tif (file.openMode() != File::OpenModeFlag::NotOpen) {\n> \t\t\tcerr << \"Valid file has invalid open mode after construction\"\n> \t\t\t << endl;\n> \t\t\treturn TestFail;\n> @@ -152,7 +152,7 @@ protected:\n> \t\t}\n>\n> \t\t/* Test open and close. */\n> -\t\tif (!file.open(File::ReadWrite)) {\n> +\t\tif (!file.open(File::OpenModeFlag::ReadWrite)) {\n> \t\t\tcerr << \"Opening file failed\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -162,7 +162,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.openMode() != File::ReadWrite) {\n> +\t\tif (file.openMode() != File::OpenModeFlag::ReadWrite) {\n> \t\t\tcerr << \"Open file has invalid open mode\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -174,7 +174,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.openMode() != File::NotOpen) {\n> +\t\tif (file.openMode() != File::OpenModeFlag::NotOpen) {\n> \t\t\tcerr << \"Closed file has invalid open mode\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -187,7 +187,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tfile.open(File::ReadOnly);\n> +\t\tfile.open(File::OpenModeFlag::ReadOnly);\n>\n> \t\tssize_t size = file.size();\n> \t\tif (size <= 0) {\n> @@ -205,12 +205,12 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (file.open(File::ReadOnly)) {\n> +\t\tif (file.open(File::OpenModeFlag::ReadOnly)) {\n> \t\t\tcerr << \"Read-only open succeeded on nonexistent file\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (!file.open(File::WriteOnly)) {\n> +\t\tif (!file.open(File::OpenModeFlag::WriteOnly)) {\n> \t\t\tcerr << \"Write-only open failed on nonexistent file\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -238,7 +238,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tfile.open(File::ReadOnly);\n> +\t\tfile.open(File::OpenModeFlag::ReadOnly);\n>\n> \t\tif (file.write(buffer) >= 0) {\n> \t\t\tcerr << \"Write succeeded on read-only file\" << endl;\n> @@ -247,7 +247,7 @@ protected:\n>\n> \t\tfile.close();\n>\n> -\t\tfile.open(File::ReadWrite);\n> +\t\tfile.open(File::OpenModeFlag::ReadWrite);\n>\n> \t\tif (file.write({ buffer.data(), 9 }) != 9) {\n> \t\t\tcerr << \"Write test failed\" << endl;\n> @@ -278,7 +278,7 @@ protected:\n>\n> \t\t/* Test mapping and unmapping. */\n> \t\tfile.setFileName(\"/proc/self/exe\");\n> -\t\tfile.open(File::ReadOnly);\n> +\t\tfile.open(File::OpenModeFlag::ReadOnly);\n>\n> \t\tSpan data = file.map();\n> \t\tif (data.empty()) {\n> @@ -316,9 +316,9 @@ protected:\n>\n> \t\t/* Test private mapping. */\n> \t\tfile.setFileName(fileName_);\n> -\t\tfile.open(File::ReadWrite);\n> +\t\tfile.open(File::OpenModeFlag::ReadWrite);\n>\n> -\t\tdata = file.map(0, -1, File::MapPrivate);\n> +\t\tdata = file.map(0, -1, File::MapFlag::Private);\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t\tif (data.empty()) {\n> \t\t\tcerr << \"Private mapping failed\" << endl;\n> \t\t\treturn TestFail;\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 C4E6CC322C\n\tfor ;\n\tMon, 26 Jul 2021 09:54:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43B02687BA;\n\tMon, 26 Jul 2021 11:54:28 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6252360272\n\tfor ;\n\tMon, 26 Jul 2021 11:54:26 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 55BA324000B;\n\tMon, 26 Jul 2021 09:54:24 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 11:55:12 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210726095512.cfsabjhjmad43jak@uno.localdomain>","References":"<20210725171827.23643-1-laurent.pinchart@ideasonboard.com>\n\t<20210725171827.23643-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210725171827.23643-6-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/5] libcamera: file: Turn MapFlag\n\tand OpenModeFlag into enum 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\" "}}]