[libcamera-devel,RFC,03/10] libcamera: File: Manage fd by ScopedFD
diff mbox series

Message ID 20210415083843.3399502-3-hiroh@chromium.org
State Changes Requested
Headers show
Series
  • Untitled series #1933
Related show

Commit Message

Hirokazu Honda April 15, 2021, 8:38 a.m. UTC 
File owns the file descriptor for a file. It should be managed
by ScopedFD to avoid the leakage.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/file.h |  5 +++--
 src/libcamera/file.cpp            | 26 +++++++++++++-------------
 2 files changed, 16 insertions(+), 15 deletions(-)

Comments

Laurent Pinchart June 6, 2021, 6:04 p.m. UTC | #1 
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@ideasonboard.com>

> Signed-off-by: Hirokazu Honda <hiroh@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 {};
Hirokazu Honda June 7, 2021, 3:22 a.m. UTC | #2 
Hi Laurent,

On Mon, Jun 7, 2021 at 3:05 AM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> 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.
>

No leakage is there today. I fixed the commit message.

Regards,
-Hiro

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > Signed-off-by: Hirokazu Honda <hiroh@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
>

Patch
diff mbox series 

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 {};