[{"id":4437,"web_url":"https://patchwork.libcamera.org/comment/4437/","msgid":"<20200413221819.GB89467@oden.dyn.berto.se>","date":"2020-04-13T22:18:19","subject":"Re: [libcamera-devel] [PATCH v2 03/11] libcamera: Add File helper\n\tclass","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your patch.\n\nOn 2020-04-13 16:30:39 +0300, Laurent Pinchart wrote:\n> The File helper class is a RAII wrapper for a file to manage the file\n> handle and memory-mapped regions.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n> Changes since v1:\n> \n> - Set error_ to 0 un successful unmap()\n> - Document error() usage for open()\n> ---\n>  src/libcamera/file.cpp            | 342 ++++++++++++++++++++++++++++++\n>  src/libcamera/include/file.h      |  69 ++++++\n>  src/libcamera/include/meson.build |   1 +\n>  src/libcamera/meson.build         |   1 +\n>  4 files changed, 413 insertions(+)\n>  create mode 100644 src/libcamera/file.cpp\n>  create mode 100644 src/libcamera/include/file.h\n> \n> diff --git a/src/libcamera/file.cpp b/src/libcamera/file.cpp\n> new file mode 100644\n> index 000000000000..8f7466099222\n> --- /dev/null\n> +++ b/src/libcamera/file.cpp\n> @@ -0,0 +1,342 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * file.cpp - File I/O operations\n> + */\n> +\n> +#include \"file.h\"\n> +\n> +#include <errno.h>\n> +#include <fcntl.h>\n> +#include <sys/mman.h>\n> +#include <sys/stat.h>\n> +#include <sys/types.h>\n> +#include <unistd.h>\n> +\n> +#include \"log.h\"\n> +\n> +/**\n> + * \\file file.h\n> + * \\brief File I/O operations\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(File);\n> +\n> +/**\n> + * \\class File\n> + * \\brief Interface for I/O operations on files\n> + *\n> + * The File class provides an interface to perform I/O operations on files. It\n> + * wraps opening, closing and mapping files in memory, and handles the cleaning\n> + * of allocated resources.\n> + *\n> + * File instances are usually constructed with a file name, but the name can be\n> + * set later through the setFileName() function. Instances are not automatically\n> + * opened when constructed, and shall be opened explictly with open().\n> + *\n> + * Files can be mapped to the process memory with map(). Mapped regions can be\n> + * unmapped manually with munmap(), and are automatically unmapped when the File\n> + * is destroyed.\n> + */\n> +\n> +/**\n> + * \\enum File::MapFlag\n> + * \\brief Flags for the File::map() function\n> + * \\var File::MapNoOption\n> + * \\brief No option (used as default value)\n> + * \\var File::MapPrivate\n> + * \\brief The memory region is mapped as private, changes are not reflected in\n> + * the file constents\n> + */\n> +\n> +/**\n> + * \\enum File::OpenMode\n> + * \\brief Mode in which a file is opened\n> + * \\var File::NotOpen\n> + * \\brief The file is not open\n> + * \\var File::ReadOnly\n> + * \\brief The file is open for reading\n> + * \\var File::WriteOnly\n> + * \\brief The file is open for writing\n> + * \\var File::ReadWrite\n> + * \\brief The file is open for reading and writing\n> + */\n> +\n> +/**\n> + * \\brief Construct a File to represent the file \\a name\n> + * \\param[in] name The file name\n> + *\n> + * Upon construction the File object is closed and shall be opened with open()\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> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a File without an associated name\n> + *\n> + * Before being used for any purpose, the file name shall be set with\n> + * setFileName().\n> + */\n> +File::File()\n> +\t: fd_(-1), mode_(NotOpen), error_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Destroy a File instance\n> + *\n> + * Any memory mapping associated with the File is unmapped, and the File is\n> + * closed if it is open.\n> + */\n> +File::~File()\n> +{\n> +\tfor (const auto &map : maps_)\n> +\t\tmunmap(map.first, map.second);\n> +\n> +\tclose();\n> +}\n> +\n> +/**\n> + * \\fn const std::string &File::fileName() const\n> + * \\brief Retrieve the file name\n> + * \\return The file name\n> + */\n> +\n> +/**\n> + * \\brief Set the name of the file\n> + * \\param[in] name The name of the file\n> + *\n> + * The \\a name can contain an absolute path, a relative path or no path at all.\n> + * Calling this function on an open file results in undefined behaviour.\n> + */\n> +void File::setFileName(const std::string &name)\n> +{\n> +\tif (isOpen()) {\n> +\t\tLOG(File, Error)\n> +\t\t\t<< \"Can't set file name on already open file \" << name_;\n> +\t\treturn;\n> +\t}\n\nWould it make sens not allowing setting the filename if maps_ is not \nempty ? I know we debated this in the previous version but never reached \na conclusion. Weather or not you think I'm too paranoid and fix this or \nnot please add,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> +\n> +\tname_ = name;\n> +}\n> +\n> +/**\n> + * \\brief Check if the file specified by fileName() exists\n> + *\n> + * This function checks if the file specified by fileName() exists. The File\n> + * instance doesn't need to be open to check for file existence, and this\n> + * function may return false even if the file is open, if it was deleted from\n> + * the file system.\n> + *\n> + * \\return True if the the file exists, false otherwise\n> + */\n> +bool File::exists() const\n> +{\n> +\treturn exists(name_);\n> +}\n> +\n> +/**\n> + * \\brief Open the file in the given mode\n> + * \\param[in] mode The open mode\n> + *\n> + * This function opens the file specified by fileName() in \\a mode. If the file\n> + * doesn't exist and the mode is WriteOnly or ReadWrite, this\n> + * function will attempt to create the file.\n> + *\n> + * The error() status is updated.\n> + *\n> + * \\return True on success, false otherwise\n> + */\n> +bool File::open(File::OpenMode mode)\n> +{\n> +\tif (isOpen()) {\n> +\t\tLOG(File, Error) << \"File \" << name_ << \" is already open\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tint flags = (mode & ReadWrite) - 1;\n> +\n> +\tfd_ = ::open(name_.c_str(), flags);\n> +\tif (fd_ < 0) {\n> +\t\terror_ = -errno;\n> +\t\treturn false;\n> +\t}\n> +\n> +\tmode_ = mode;\n> +\terror_ = 0;\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\fn bool File::isOpen() const\n> + * \\brief Check if the file is open\n> + * \\return True if the file is open, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn OpenMode File::openMode() const\n> + * \\brief Retrieve the file open mode\n> + * \\return The file open mode\n> + */\n> +\n> +/**\n> + * \\brief Close the file\n> + *\n> + * This function closes the File. If the File is not open, it performs no\n> + * operation. Memory mappings created with map() are not destroyed when the\n> + * file is closed.\n> + */\n> +void File::close()\n> +{\n> +\tif (fd_ == -1)\n> +\t\treturn;\n> +\n> +\t::close(fd_);\n> +\tfd_ = -1;\n> +\tmode_ = NotOpen;\n> +}\n> +\n> +/**\n> + * \\fn int File::error() const\n> + * \\brief Retrieve the file error status\n> + *\n> + * This function retrieves the error status from the last file open or I/O\n> + * operation. The error status is a negative number as defined by errno.h. If\n> + * no error occurred, this function returns 0.\n> + *\n> + * \\return The file error status\n> + */\n> +\n> +/**\n> + * \\brief Retrieve the file size\n> + *\n> + * This function retrieves the size of the file on the filesystem. The File\n> + * instance shall be open to retrieve its size. The error() status is not\n> + * modified, error codes are returned directly on failure.\n> + *\n> + * \\return The file size in bytes on success, or a negative error code otherwise\n> + */\n> +ssize_t File::size() const\n> +{\n> +\tif (!isOpen())\n> +\t\treturn -EINVAL;\n> +\n> +\tstruct stat st;\n> +\tint ret = fstat(fd_, &st);\n> +\tif (ret < 0)\n> +\t\treturn -errno;\n> +\n> +\treturn st.st_size;\n> +}\n> +\n> +/**\n> + * \\brief Map a region of the file in the process memory\n> + * \\param[in] offset The region offset within the file\n> + * \\param[in] size The region sise\n> + * \\param[in] flags The mapping flags\n> + *\n> + * This function maps a region of \\a size bytes of the file starting at \\a\n> + * offset into the process memory. The File instance shall be open, but may be\n> + * closed after mapping the region. Mappings stay valid when the File is\n> + * closed, and are destroyed automatically when the File is deleted.\n> + *\n> + * If \\a size is a negative value, this function maps the region starting at \\a\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> + *\n> + * The error() status is updated.\n> + *\n> + * \\return The mapped memory on success, or an empty span otherwise\n> + */\n> +Span<uint8_t> File::map(off_t offset, ssize_t size, enum File::MapFlag flags)\n> +{\n> +\tif (!isOpen()) {\n> +\t\terror_ = -EBADF;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tif (size < 0) {\n> +\t\tsize = File::size();\n> +\t\tif (size < 0) {\n> +\t\t\terror_ = size;\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tsize -= offset;\n> +\t}\n> +\n> +\tint mmapFlags = flags & MapPrivate ? MAP_PRIVATE : MAP_SHARED;\n> +\n> +\tint prot = 0;\n> +\tif (mode_ & ReadOnly)\n> +\t\tprot |= PROT_READ;\n> +\tif (mode_ & WriteOnly)\n> +\t\tprot |= PROT_WRITE;\n> +\tif (flags & MapPrivate)\n> +\t\tprot |= PROT_WRITE;\n> +\n> +\tvoid *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);\n> +\tif (map == MAP_FAILED) {\n> +\t\terror_ = -errno;\n> +\t\treturn {};\n> +\t}\n> +\n> +\tmaps_.emplace(map, size);\n> +\n> +\terror_ = 0;\n> +\treturn { static_cast<uint8_t *>(map), static_cast<size_t>(size) };\n> +}\n> +\n> +/**\n> + * \\brief Unmap a region mapped with map()\n> + * \\param[in] addr The region address\n> + *\n> + * The error() status is updated.\n> + *\n> + * \\return True on success, or false if an error occurs\n> + */\n> +bool File::unmap(uint8_t *addr)\n> +{\n> +\tauto iter = maps_.find(static_cast<void *>(addr));\n> +\tif (iter == maps_.end()) {\n> +\t\terror_ = -ENOENT;\n> +\t\treturn false;\n> +\t}\n> +\n> +\tint ret = munmap(addr, iter->second);\n> +\tif (ret < 0) {\n> +\t\terror_ = -errno;\n> +\t\treturn false;\n> +\t}\n> +\n> +\tmaps_.erase(iter);\n> +\n> +\terror_ = 0;\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Check if the file specified by \\a name exists\n> + * \\param[in] name The file name\n> + * \\return True if the file exists, false otherwise\n> + */\n> +bool File::exists(const std::string &name)\n> +{\n> +\tstruct stat st;\n> +\tint ret = stat(name.c_str(), &st);\n> +\tif (ret < 0)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/file.h b/src/libcamera/include/file.h\n> new file mode 100644\n> index 000000000000..ea6f121cb6c5\n> --- /dev/null\n> +++ b/src/libcamera/include/file.h\n> @@ -0,0 +1,69 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * file.h - File I/O operations\n> + */\n> +#ifndef __LIBCAMERA_FILE_H__\n> +#define __LIBCAMERA_FILE_H__\n> +\n> +#include <map>\n> +#include <string>\n> +#include <sys/types.h>\n> +\n> +#include <libcamera/span.h>\n> +\n> +namespace libcamera {\n> +\n> +class File\n> +{\n> +public:\n> +\tenum MapFlag {\n> +\t\tMapNoOption = 0,\n> +\t\tMapPrivate = (1 << 0),\n> +\t};\n> +\n> +\tenum OpenMode {\n> +\t\tNotOpen = 0,\n> +\t\tReadOnly = (1 << 0),\n> +\t\tWriteOnly = (1 << 1),\n> +\t\tReadWrite = ReadOnly | WriteOnly,\n> +\t};\n> +\n> +\tFile(const std::string &name);\n> +\tFile();\n> +\t~File();\n> +\n> +\tFile(const File &) = delete;\n> +\tFile &operator=(const File &) = delete;\n> +\n> +\tconst std::string &fileName() const { return name_; }\n> +\tvoid setFileName(const std::string &name);\n> +\tbool exists() const;\n> +\n> +\tbool open(OpenMode mode);\n> +\tbool isOpen() const { return fd_ != -1; }\n> +\tOpenMode openMode() const { return mode_; }\n> +\tvoid close();\n> +\n> +\tint error() const { return error_; }\n> +\tssize_t size() const;\n> +\n> +\tSpan<uint8_t> map(off_t offset = 0, ssize_t size = -1,\n> +\t\t\t  MapFlag flags = MapNoOption);\n> +\tbool unmap(uint8_t *addr);\n> +\n> +\tstatic bool exists(const std::string &name);\n> +\n> +private:\n> +\tstd::string name_;\n> +\tint fd_;\n> +\tOpenMode mode_;\n> +\n> +\tint error_;\n> +\tstd::map<void *, size_t> maps_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_FILE_H__ */\n> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build\n> index 17e2bed93fba..921ed5a063cb 100644\n> --- a/src/libcamera/include/meson.build\n> +++ b/src/libcamera/include/meson.build\n> @@ -8,6 +8,7 @@ libcamera_headers = files([\n>      'device_enumerator_sysfs.h',\n>      'device_enumerator_udev.h',\n>      'event_dispatcher_poll.h',\n> +    'file.h',\n>      'formats.h',\n>      'ipa_context_wrapper.h',\n>      'ipa_manager.h',\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 87fa09cde63d..4f5c41678781 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -14,6 +14,7 @@ libcamera_sources = files([\n>      'event_dispatcher.cpp',\n>      'event_dispatcher_poll.cpp',\n>      'event_notifier.cpp',\n> +    'file.cpp',\n>      'file_descriptor.cpp',\n>      'formats.cpp',\n>      'framebuffer_allocator.cpp',\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7D4660407\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 14 Apr 2020 00:18:21 +0200 (CEST)","by mail-lf1-x12b.google.com with SMTP id u10so6642043lfo.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 13 Apr 2020 15:18:21 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tl13sm7891627ljc.84.2020.04.13.15.18.19\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 13 Apr 2020 15:18:19 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"rt3rBKZt\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=LmlEYO77OpLfH121C1vl7HgsiUHF/7vdO6OHmnLdLB0=;\n\tb=rt3rBKZtze4SfKSdL63cqIdYDGhy+ghAWYm6InH/oS5Qu/HCUSsgcvvt0Xd03MP8Lp\n\trScBujPzb7jrtQlNwCxnDKE3/7P2UqjVI+H0fTN/i3EcT3FBon0z1GCILTg+FVGDGF+s\n\tNP+lJ3XTuCrRpXRTecG6cfxjKh+PeIdBBx2w72Gz2Cl+Uv14oVdOJFgsWPjzPN3OXd5D\n\t6HjHrox1LFktWZVTZ4vc0ddJbt7/tAq1+IqtswawBFUSZ2wR4/TjZMAso2YyIu/dW5Dh\n\tEmt/0cgzmWhEKaKYS0mPN07rCZ9yr9feGAfCwxRSH2ZehvNjoMb80Sehiqfn+AmxIFRE\n\teBoQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=LmlEYO77OpLfH121C1vl7HgsiUHF/7vdO6OHmnLdLB0=;\n\tb=jJkHRopfEZuDy7Pxp7oVUSRvq3VzxyKg0kn82zlXgUTTgEYRgZkeGPVCJLCiH2KnSJ\n\t865OjLYOIG6HdQyy3Jk3Grwi5oN1H0v4ljLx0HNaZkNguexZ/kk3x4DgCI6zRvS6ZtUs\n\tBEAiJ65bgeu+1sRFeiMkqsPGcdeYlW7HP5MUa+uXkdvBXZhTPOgjH5LxQpmJ2Un93Uzw\n\tbUfhRxOi9cT+ifJLYmz6NFq7KjyK0AwUyhFC7kym4njsdjrlgEvpD7mTluaIShI0+CPb\n\tc/EwRuZK+GSAGHtyngKEfcPmU96KVi94qtvWhJAZefSNfd3S4HcXdgpbMKOx9+Q2DIik\n\t+bJw==","X-Gm-Message-State":"AGi0PubnWExWQEB6UIsJEoJdz9A9b/W2cfAlqfUN7QN0hWSJBLa5ZoYJ\n\tLiY7nZmQO8JD4/DDgFnFdZ60vw==","X-Google-Smtp-Source":"APiQypICAG5SJDlrVlFnbHgl9vnisOeWt6tTIbrq3A9uFs7NWMviqkZxe0HlWjwR0NgvpF6p4TjbPw==","X-Received":"by 2002:a19:700b:: with SMTP id h11mr5004620lfc.89.1586816300856;\n\tMon, 13 Apr 2020 15:18:20 -0700 (PDT)","Date":"Tue, 14 Apr 2020 00:18:19 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200413221819.GB89467@oden.dyn.berto.se>","References":"<20200413133047.11913-1-laurent.pinchart@ideasonboard.com>\n\t<20200413133047.11913-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200413133047.11913-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 03/11] libcamera: Add File helper\n\tclass","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 13 Apr 2020 22:18:22 -0000"}}]