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

[Kieran Bingham]

On 12/07/2020 20:32, Kieran Bingham wrote:
> 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.

C) Call lseek directly to obtain the actual pos...
D) Remove pos_ and follow neg's suggestion ;-)

Even still, once a route forwards on that is addressed this still holds:

> 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