[libcamera-devel] [RFC PATCH 03/10] libcamera: File: Manage fd by ScopedFD

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 15, 2021 at 05:38:36PM +0900, Hirokazu Honda wrote:
> File owns the file descriptor for a file. It should be managed
> by ScopedFD to avoid the leakage.

Is there any leak in the current implementation ? If not, I'd reword the
commit message.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/file.h |  5 +++--
>  src/libcamera/file.cpp            | 26 +++++++++++++-------------
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/internal/file.h b/include/libcamera/internal/file.h
> index f0b313a5..e6eb7f04 100644
> --- a/include/libcamera/internal/file.h
> +++ b/include/libcamera/internal/file.h
> @@ -12,6 +12,7 @@
>  #include <sys/types.h>
>  
>  #include <libcamera/class.h>
> +#include <libcamera/scoped_file_descriptor.h>
>  #include <libcamera/span.h>
>  
>  namespace libcamera {
> @@ -40,7 +41,7 @@ public:
>  	bool exists() const;
>  
>  	bool open(OpenMode mode);
> -	bool isOpen() const { return fd_ != -1; }
> +	bool isOpen() const { return fd_.isValid(); }
>  	OpenMode openMode() const { return mode_; }
>  	void close();
>  
> @@ -65,7 +66,7 @@ private:
>  	void unmapAll();
>  
>  	std::string name_;
> -	int fd_;
> +	ScopedFD fd_;
>  	OpenMode mode_;
>  
>  	int error_;
> diff --git a/src/libcamera/file.cpp b/src/libcamera/file.cpp
> index bce2b613..4db9a8d3 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), mode_(NotOpen), error_(0)
>  {
>  }
>  
> @@ -84,7 +84,7 @@ File::File(const std::string &name)
>   * setFileName().
>   */
>  File::File()
> -	: fd_(-1), mode_(NotOpen), error_(0)
> +	: mode_(NotOpen), error_(0)
>  {
>  }
>  
> @@ -167,12 +167,13 @@ bool File::open(File::OpenMode mode)
>  	if (mode & WriteOnly)
>  		flags |= O_CREAT;
>  
> -	fd_ = ::open(name_.c_str(), flags, 0666);
> -	if (fd_ < 0) {
> +	int fd = ::open(name_.c_str(), flags, 0666);
> +	if (fd < 0) {
>  		error_ = -errno;
>  		return false;
>  	}
>  
> +	fd_ = ScopedFD(fd);
>  	mode_ = mode;
>  	error_ = 0;
>  	return true;
> @@ -199,11 +200,10 @@ bool File::open(File::OpenMode mode)
>   */
>  void File::close()
>  {
> -	if (fd_ == -1)
> +	if (!fd_.isValid())
>  		return;
>  
> -	::close(fd_);
> -	fd_ = -1;
> +	fd_.reset();
>  	mode_ = NotOpen;
>  }
>  
> @@ -233,7 +233,7 @@ ssize_t File::size() const
>  		return -EINVAL;
>  
>  	struct stat st;
> -	int ret = fstat(fd_, &st);
> +	int ret = fstat(fd_.get(), &st);
>  	if (ret < 0)
>  		return -errno;
>  
> @@ -252,7 +252,7 @@ off_t File::pos() const
>  	if (!isOpen())
>  		return 0;
>  
> -	return lseek(fd_, 0, SEEK_CUR);
> +	return lseek(fd_.get(), 0, SEEK_CUR);
>  }
>  
>  /**
> @@ -266,7 +266,7 @@ off_t File::seek(off_t pos)
>  	if (!isOpen())
>  		return -EINVAL;
>  
> -	off_t ret = lseek(fd_, pos, SEEK_SET);
> +	off_t ret = lseek(fd_.get(), pos, SEEK_SET);
>  	if (ret < 0)
>  		return -errno;
>  
> @@ -298,7 +298,7 @@ ssize_t File::read(const Span<uint8_t> &data)
>  
>  	/* Retry in case of interrupted system calls. */
>  	while (readBytes < data.size()) {
> -		ret = ::read(fd_, data.data() + readBytes,
> +		ret = ::read(fd_.get(), data.data() + readBytes,
>  			     data.size() - readBytes);
>  		if (ret <= 0)
>  			break;
> @@ -335,7 +335,7 @@ ssize_t File::write(const Span<const uint8_t> &data)
>  
>  	/* Retry in case of interrupted system calls. */
>  	while (writtenBytes < data.size()) {
> -		ssize_t ret = ::write(fd_, data.data() + writtenBytes,
> +		ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,
>  				      data.size() - writtenBytes);
>  		if (ret <= 0)
>  			break;
> @@ -398,7 +398,7 @@ Span<uint8_t> File::map(off_t offset, ssize_t size, enum File::MapFlag flags)
>  	if (flags & MapPrivate)
>  		prot |= PROT_WRITE;
>  
> -	void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);
> +	void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);
>  	if (map == MAP_FAILED) {
>  		error_ = -errno;
>  		return {};

-- 
Regards,

Laurent Pinchart