[libcamera-devel] [PATCH 03/11] libcamera: Add File helper class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Apr 7 22:19:34 CEST 2020
Hi Laurent,
Thanks for your work.
On 2020-04-04 04:56:16 +0300, Laurent Pinchart wrote:
> The File helper class is a RAII wrapper for a file to manage the file
> handle and memory-mapped regions.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/file.cpp | 338 ++++++++++++++++++++++++++++++
> src/libcamera/include/file.h | 69 ++++++
> src/libcamera/include/meson.build | 1 +
> src/libcamera/meson.build | 1 +
> 4 files changed, 409 insertions(+)
> create mode 100644 src/libcamera/file.cpp
> create mode 100644 src/libcamera/include/file.h
>
> diff --git a/src/libcamera/file.cpp b/src/libcamera/file.cpp
> new file mode 100644
> index 000000000000..23cea4aa3101
> --- /dev/null
> +++ b/src/libcamera/file.cpp
> @@ -0,0 +1,338 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * file.cpp - File I/O operations
> + */
> +
> +#include "file.h"
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "log.h"
> +
> +/**
> + * \file file.h
> + * \brief File I/O operations
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(File);
> +
> +/**
> + * \class File
> + * \brief Interface for I/O operations on files
> + *
> + * The File class provides an interface to perform I/O operations on files. It
> + * wraps opening, closing and mapping files in memory, and handles the cleaning
> + * of allocated resources.
> + *
> + * File instances are usually constructed with a file name, but the name can be
> + * set later through the setFileName() function. Instances are not automatically
> + * opened when constructed, and shall be opened explictly with open().
> + *
> + * Files can be mapped to the process memory with map(). Mapped regions can be
> + * unmapped manually with munmap(), and are automatically unmapped when the File
> + * is destroyed.
> + */
> +
> +/**
> + * \enum File::MapFlag
> + * \brief Flags for the File::map() function
> + * \var File::MapNoOption
> + * \brief No option (used as default value)
> + * \var File::MapPrivate
> + * \brief The memory region is mapped as private, changes are not reflected in
> + * the file constents
> + */
> +
> +/**
> + * \enum File::OpenMode
> + * \brief Mode in which a file is opened
> + * \var File::NotOpen
> + * \brief The file is not open
> + * \var File::ReadOnly
> + * \brief The file is open for reading
> + * \var File::WriteOnly
> + * \brief The file is open for writing
> + * \var File::ReadWrite
> + * \brief The file is open for reading and writing
> + */
> +
> +/**
> + * \brief Construct a File to represent the file \a name
> + * \param[in] name The file name
> + *
> + * Upon construction the File object is closed and shall be opened with open()
> + * before performing I/O operations.
> + */
> +File::File(const std::string &name)
> + : name_(name), fd_(-1), mode_(NotOpen), error_(0)
> +{
> +}
> +
> +/**
> + * \brief Construct a File without an associated name
> + *
> + * Before being used for any purpose, the file name shall be set with
> + * setFileName().
> + */
> +File::File()
> + : fd_(-1), mode_(NotOpen), error_(0)
> +{
> +}
> +
> +/**
> + * \brief Destroy a File instance
> + *
> + * Any memory mapping associated with the File is unmapped, and the File is
> + * closed if it is open.
> + */
> +File::~File()
> +{
> + for (const auto &map : maps_)
> + munmap(map.first, map.second);
> +
> + close();
> +}
> +
> +/**
> + * \fn const std::string &File::fileName() const
> + * \brief Retrieve the file name
> + * \return The file name
> + */
> +
> +/**
> + * \brief Set the name of the file
> + * \param[in] name The name of the file
> + *
> + * The \a name can contain an absolute path, a relative path or no path at all.
> + * Calling this function on an open file results in undefined behaviour.
> + */
> +void File::setFileName(const std::string &name)
> +{
> + if (isOpen()) {
> + LOG(File, Error)
> + << "Can't set file name on already open file " << name_;
> + return;
> + }
> +
> + name_ = name;
> +}
> +
> +/**
> + * \brief Check if the file specified by fileName() exists
> + *
> + * This function checks if the file specified by fileName() exists. The File
> + * instance doesn't need to be open to check for file existence, and this
> + * function may return false even if the file is open, if it was deleted from
> + * the file system.
> + *
> + * \return True if the the file exists, false otherwise
> + */
> +bool File::exists() const
> +{
> + return exists(name_);
> +}
> +
> +/**
> + * \brief Open the file in the given mode
> + * \param[in] mode The open mode
> + *
> + * This function opens the file specified by fileName() in \a mode. If the file
> + * doesn't exist and the mode is WriteOnly or ReadWrite, this
> + * function will attempt to create the file.
Shall the updated error status be mentioned here ?
> + *
> + * \return True on success, false otherwise
> + */
> +bool File::open(File::OpenMode mode)
> +{
> + if (isOpen()) {
> + LOG(File, Error) << "File " << name_ << " is already open";
> + return false;
> + }
> +
> + int flags = (mode & ReadWrite) - 1;
> +
> + fd_ = ::open(name_.c_str(), flags);
> + if (fd_ < 0) {
> + error_ = -errno;
> + return false;
> + }
> +
> + mode_ = mode;
> + error_ = 0;
> + return true;
> +}
> +
> +/**
> + * \fn bool File::isOpen() const
> + * \brief Check if the file is open
> + * \return True if the file is open, false otherwise
> + */
> +
> +/**
> + * \fn OpenMode File::openMode() const
> + * \brief Retrieve the file open mode
> + * \return The file open mode
> + */
> +
> +/**
> + * \brief Close the file
> + *
> + * This function closes the File. If the File is not open, it performs no
> + * operation. Memory mappings created with map() are not destroyed when the
> + * file is closed.
> + */
> +void File::close()
> +{
> + if (fd_ == -1)
> + return;
> +
> + ::close(fd_);
> + fd_ = -1;
> + mode_ = NotOpen;
> +}
> +
> +/**
> + * \fn int File::error() const
> + * \brief Retrieve the file error status
> + *
> + * This function retrieves the error status from the last file open or I/O
> + * operation. The error status is a negative number as defined by errno.h. If
> + * no error occurred, this function returns 0.
> + *
> + * \return The file error status
> + */
> +
> +/**
> + * \brief Retrieve the file size
> + *
> + * This function retrieves the size of the file on the filesystem. The File
> + * instance shall be open to retrieve its size. The error() status is not
> + * modified, error codes are returned directly on failure.
> + *
> + * \return The file size in bytes on success, or a negative error code otherwise
> + */
> +ssize_t File::size() const
> +{
> + if (!isOpen())
> + return -EINVAL;
> +
> + struct stat st;
> + int ret = fstat(fd_, &st);
> + if (ret < 0)
> + return -errno;
> +
> + return st.st_size;
> +}
> +
> +/**
> + * \brief Map a region of the file in the process memory
> + * \param[in] offset The region offset within the file
> + * \param[in] size The region sise
> + * \param[in] flags The mapping flags
> + *
> + * This function maps a region of \a size bytes of the file starting at \a
> + * offset into the process memory. The File instance shall be open, but may be
> + * closed after mapping the region. Mappings stay valid when the File is
> + * closed, and are destroyed automatically when the File is deleted.
I find this a bit dangerous. Would it not be confusing if we allowed a
File to be used to open and map /foo/bar, then close it open and map
/foo/baz and both mappings are removed when the single a signle File
object is destroyed ?
> + *
> + * If \a size is a negative value, this function maps the region starting at \a
> + * offset until the end of the file.
> + *
> + * The mapping memory protection is controlled by the file open mode, unless \a
> + * flags contains MapPrivate in which case the region is mapped in read/write
> + * mode.
> + *
> + * The error() status is updated.
> + *
> + * \return The mapped memory on success, or an empty span otherwise
> + */
> +Span<uint8_t> File::map(off_t offset, ssize_t size, enum File::MapFlag flags)
> +{
> + if (!isOpen()) {
> + error_ = -EBADF;
> + return {};
> + }
> +
> + if (size < 0) {
> + size = File::size();
> + if (size < 0) {
> + error_ = size;
> + return {};
> + }
> +
> + size -= offset;
> + }
> +
> + int mmapFlags = flags & MapPrivate ? MAP_PRIVATE : MAP_SHARED;
> +
> + int prot = 0;
> + if (mode_ & ReadOnly)
> + prot |= PROT_READ;
> + if (mode_ & WriteOnly)
> + prot |= PROT_WRITE;
> + if (flags & MapPrivate)
> + prot |= PROT_WRITE;
> +
> + void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);
> + if (map == MAP_FAILED) {
> + error_ = -errno;
> + return {};
> + }
> +
> + maps_.emplace(map, size);
> +
> + error_ = 0;
> + return { static_cast<uint8_t *>(map), static_cast<size_t>(size) };
> +}
> +
> +/**
> + * \brief Unmap a region mapped with map()
> + * \param[in] addr The region address
> + *
> + * The error() status is updated.
> + *
> + * \return True on success, or false if an error occurs
> + */
> +bool File::unmap(uint8_t *addr)
> +{
> + auto iter = maps_.find(static_cast<void *>(addr));
> + if (iter == maps_.end()) {
> + error_ = -ENOENT;
> + return false;
> + }
> +
> + int ret = munmap(addr, iter->second);
> + if (ret < 0) {
> + error_ = -errno;
> + return false;
> + }
> +
> + maps_.erase(iter);
I think error_ shall be set to 0 here right?
> + return true;
> +}
> +
> +/**
> + * \brief Check if the file specified by \a name exists
> + * \param[in] name The file name
> + * \return True if the file exists, false otherwise
> + */
> +bool File::exists(const std::string &name)
> +{
> + struct stat st;
> + int ret = stat(name.c_str(), &st);
> + if (ret < 0)
> + return false;
> +
> + return true;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/file.h b/src/libcamera/include/file.h
> new file mode 100644
> index 000000000000..ea6f121cb6c5
> --- /dev/null
> +++ b/src/libcamera/include/file.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * file.h - File I/O operations
> + */
> +#ifndef __LIBCAMERA_FILE_H__
> +#define __LIBCAMERA_FILE_H__
> +
> +#include <map>
> +#include <string>
> +#include <sys/types.h>
> +
> +#include <libcamera/span.h>
> +
> +namespace libcamera {
> +
> +class File
> +{
> +public:
> + enum MapFlag {
> + MapNoOption = 0,
> + MapPrivate = (1 << 0),
> + };
> +
> + enum OpenMode {
> + NotOpen = 0,
> + ReadOnly = (1 << 0),
> + WriteOnly = (1 << 1),
> + ReadWrite = ReadOnly | WriteOnly,
> + };
> +
> + File(const std::string &name);
> + File();
> + ~File();
> +
> + File(const File &) = delete;
> + File &operator=(const File &) = delete;
> +
> + const std::string &fileName() const { return name_; }
> + void setFileName(const std::string &name);
> + bool exists() const;
> +
> + bool open(OpenMode mode);
The API seems a bit odd here.
When opening a file we are required to provide how to open it but not
which file to open. Would this class be simplified if the file to open
would also be provided as an argument to open instead of either to a
constructor or setFileName() ?
> + bool isOpen() const { return fd_ != -1; }
> + OpenMode openMode() const { return mode_; }
> + void close();
> +
> + int error() const { return error_; }
> + ssize_t size() const;
> +
> + Span<uint8_t> map(off_t offset = 0, ssize_t size = -1,
> + MapFlag flags = MapNoOption);
> + bool unmap(uint8_t *addr);
Would it make sens to pass the Span<uint8_t> returned from map as an
argument to unmap? Reading the class definition suggest their is an
unbalance between new fancy stuff and plain old data ;-)
> +
> + static bool exists(const std::string &name);
> +
> +private:
> + std::string name_;
> + int fd_;
> + OpenMode mode_;
> +
> + int error_;
> + std::map<void *, size_t> maps_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_FILE_H__ */
> diff --git a/src/libcamera/include/meson.build b/src/libcamera/include/meson.build
> index 17e2bed93fba..921ed5a063cb 100644
> --- a/src/libcamera/include/meson.build
> +++ b/src/libcamera/include/meson.build
> @@ -8,6 +8,7 @@ libcamera_headers = files([
> 'device_enumerator_sysfs.h',
> 'device_enumerator_udev.h',
> 'event_dispatcher_poll.h',
> + 'file.h',
> 'formats.h',
> 'ipa_context_wrapper.h',
> 'ipa_manager.h',
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 87fa09cde63d..4f5c41678781 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -14,6 +14,7 @@ libcamera_sources = files([
> 'event_dispatcher.cpp',
> 'event_dispatcher_poll.cpp',
> 'event_notifier.cpp',
> + 'file.cpp',
> 'file_descriptor.cpp',
> 'formats.cpp',
> 'framebuffer_allocator.cpp',
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list