[libcamera-devel] [PATCH 1/4] libcamera: file: Add read/write support
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 14 09:59:03 CEST 2020
Hi,
On 14/07/2020 06:52, Niklas Söderlund wrote:
> Hi Laurent,
>
> On 2020-07-14 01:55:36 +0300, Laurent Pinchart wrote:
>> Hi Niklas, Kieran,
>>
>> On Mon, Jul 13, 2020 at 09:31:42AM +0100, Kieran Bingham wrote:
>>> Hi Niklas / Laurent,
>>>
>>> On 13/07/2020 07:53, Niklas Söderlund wrote:
>>>> Hi Laurent,
>>>>
>>>> Thanks for your work.
>>>>
>>>> On 2020-07-12 17:44:16 +0300, 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_; }
>>>>
>>>> Would it not be nicer to return lseek(fd_, 0, SEEK_CUR) here? That way
>>>> we could avoid keeping pos_ in sync in potential corner cases.
>>>
>>> Aha ... this would of course address my concerns about pos_ becoming
>>> invalid on error cases ...
>>
>> Wouldn't it be more efficient to update pos_ in case of a read or write
>> error ? Efficiency may not be our primary concern here though. I don't
>> mind much either way, does anyone have a preference ?
>
> No strong preference, but if the kernel keeps track of things for you it
> seems folly to reimplement it in user-space :-)
>
> Whatever counters the kernel keep to make SEEK_CUR work it will still do
> disregarding if we use it or not. So the only efficiency gain would be
> if we read it often and modified it a seldom, in that case I rather just
> cache SEEK_CUR and refresh it each time we to a write() or read().
My feel is that it's keeping a cached value of the position in
userspace, which is rarely (if ever) used, and not expected to be a
performance gain to cache it, /and/ it can be wrong in error cases, and
thus need updating.
IMO, it's likely adding code for the sake of it.
I'd go for just a direct call to lseek to obtain pos() when requested,
which is probably just a helper and doesn't have any callers anyway?
If that ever became a performance bottle neck, it could be cached, but I
really can't see that being a case we're going to experience in this
instance.
Do we even need to provide pos() at all?
I don't mind adding it as a generic API function on the libcamera::File
API, but it's internal anyway?
Perhaps we'll see more use of the libcamera::File api for
reading/writing configuration files ... is that where you anticipate
needing to obtain the current pos()?
--
Kieran
>>>>> + 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)
>>>>> + 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,
>>
>> Laurent Pinchart
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list