[libcamera-devel] [PATCH 1/4] libcamera: file: Add read/write support

[Kieran Bingham]

Hi Laurent,

On 12/07/2020 15:44, Laurent Pinchart wrote:
> Add basic support to read and write data from/to a file, along with
> retrieving and setting the current read/write position.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/internal/file.h |   7 ++
>  src/libcamera/file.cpp            | 111 +++++++++++++++++++++++++++++-
>  2 files changed, 116 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/internal/file.h b/include/libcamera/internal/file.h
> index e3e72132e336..ead94cdf29c9 100644
> --- a/include/libcamera/internal/file.h
> +++ b/include/libcamera/internal/file.h
> @@ -49,6 +49,12 @@ public:
>  	int error() const { return error_; }
>  	ssize_t size() const;
>  
> +	off_t pos() const { return pos_; }
> +	off_t seek(off_t pos);
> +
> +	ssize_t read(const Span<uint8_t> &data);
> +	ssize_t write(const Span<const uint8_t> &data);
> +
>  	Span<uint8_t> map(off_t offset = 0, ssize_t size = -1,
>  			  MapFlag flags = MapNoOption);
>  	bool unmap(uint8_t *addr);
> @@ -61,6 +67,7 @@ private:
>  	std::string name_;
>  	int fd_;
>  	OpenMode mode_;
> +	off_t pos_;
>  
>  	int error_;
>  	std::map<void *, size_t> maps_;
> diff --git a/src/libcamera/file.cpp b/src/libcamera/file.cpp
> index c471bde3fc68..8bd109090919 100644
> --- a/src/libcamera/file.cpp
> +++ b/src/libcamera/file.cpp
> @@ -73,7 +73,7 @@ LOG_DEFINE_CATEGORY(File);
>   * before performing I/O operations.
>   */
>  File::File(const std::string &name)
> -	: name_(name), fd_(-1), mode_(NotOpen), error_(0)
> +	: name_(name), fd_(-1), mode_(NotOpen), pos_(0), error_(0)
>  {
>  }
>  
> @@ -84,7 +84,7 @@ File::File(const std::string &name)
>   * setFileName().
>   */
>  File::File()
> -	: fd_(-1), mode_(NotOpen), error_(0)
> +	: fd_(-1), mode_(NotOpen), pos_(0), error_(0)
>  {
>  }
>  
> @@ -202,6 +202,7 @@ void File::close()
>  	::close(fd_);
>  	fd_ = -1;
>  	mode_ = NotOpen;
> +	pos_ = 0;
>  }
>  
>  /**
> @@ -237,6 +238,112 @@ ssize_t File::size() const
>  	return st.st_size;
>  }
>  
> +/**
> + * \fn off_t File::pos() const
> + * \brief Return current read or write position
> + *
> + * If the file is closed, this function returns 0.
> + *
> + * \return The current read or write position
> + */
> +
> +/**
> + * \brief Set the read or write position
> + * \param[in] pos The desired position
> + * \return The resulting offset from the beginning of the file on success, or a
> + * negative error code otherwise
> + */
> +off_t File::seek(off_t pos)
> +{
> +	if (!isOpen())
> +		return -EINVAL;
> +
> +	off_t ret = lseek(fd_, pos, SEEK_SET);
> +	if (ret < 0)
> +		return -errno;
> +
> +	pos_ = ret;
> +	return ret;
> +}
> +
> +/**
> + * \brief Read data from the file
> + * \param[in] data Memory to read data into
> + *
> + * Read at most \a data.size() bytes from the file into \a data.data(), and
> + * return the number of bytes read. If less data than requested is available,
> + * the returned byte count may be smaller than the requested size. If no more
> + * data is available, 0 is returned.
> + *
> + * The position of the file as returned by pos() is advanced by the number of
> + * bytes read. If an error occurs, the position isn't modified.
> + *
> + * \return The number of bytes read on success, or a negative error code
> + * otherwise
> + */
> +ssize_t File::read(const Span<uint8_t> &data)
> +{
> +	if (!isOpen())
> +		return -EINVAL;
> +
> +	size_t readBytes = 0;
> +	ssize_t ret = 0;
> +
> +	/* Retry in case of interrupted system calls. */
> +	while (readBytes < data.size()) {
> +		ret = ::read(fd_, data.data() + readBytes,
> +			     data.size() - readBytes);
> +		if (ret <= 0)
> +			break;
> +
> +		readBytes += ret;
> +	}
> +
> +	if (ret < 0 && !readBytes)

the man page for read(2) states:


# On  error,  -1  is returned, and errno is set appropriately.  In this
# case, it is left unspecified whether the file position (if any)
# changes.


In the event of an error, that could thus leave pos_ invalid, but I
don't think we care too much about that for an internal helper which
probably isn't going to use pos much anyway.

We could:

A) call lseek to explicitly reset pos
B) Continue to do nothing.

I think we'll choose option B for now, especially as the function
documentation for both explicitly states that the pos will not be moved
on error. ;-)


So ...

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +		return -errno;
> +
> +	pos_ += readBytes;
> +	return readBytes;
> +}
> +
> +/**
> + * \brief Write data to the file
> + * \param[in] data Memory containing data to be written
> + *
> + * Write at most \a data.size() bytes from \a data.data() to the file, and
> + * return the number of bytes written. If the file system doesn't have enough
> + * space for the data, the returned byte count may be less than requested.
> + *
> + * The position of the file as returned by pos() is advanced by the number of
> + * bytes written. If an error occurs, the position isn't modified.
> + *
> + * \return The number of bytes written on success, or a negative error code
> + * otherwise
> + */
> +ssize_t File::write(const Span<const uint8_t> &data)
> +{
> +	if (!isOpen())
> +		return -EINVAL;
> +
> +	size_t writtenBytes = 0;
> +
> +	/* Retry in case of interrupted system calls. */
> +	while (writtenBytes < data.size()) {
> +		ssize_t ret = ::write(fd_, data.data() + writtenBytes,
> +				      data.size() - writtenBytes);
> +		if (ret <= 0)
> +			break;
> +
> +		writtenBytes += ret;
> +	}
> +
> +	if (data.size() && !writtenBytes)
> +		return -errno;
> +
> +	pos_ += writtenBytes;> +	return writtenBytes;
> +}
> +
>  /**
>   * \brief Map a region of the file in the process memory
>   * \param[in] offset The region offset within the file
> 

-- 
Regards
--
Kieran