<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 7, 2021 at 3:05 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Apr 15, 2021 at 05:38:36PM +0900, Hirokazu Honda wrote:<br>
> File owns the file descriptor for a file. It should be managed<br>
> by ScopedFD to avoid the leakage.<br>
<br>
Is there any leak in the current implementation ? If not, I'd reword the<br>
commit message.<br></blockquote><div><br></div><div>No leakage is there today. I fixed the commit message.</div><div><br></div><div>Regards,</div><div>-Hiro </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> include/libcamera/internal/file.h | 5 +++--<br>
> src/libcamera/file.cpp | 26 +++++++++++++-------------<br>
> 2 files changed, 16 insertions(+), 15 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/internal/file.h b/include/libcamera/internal/file.h<br>
> index f0b313a5..e6eb7f04 100644<br>
> --- a/include/libcamera/internal/file.h<br>
> +++ b/include/libcamera/internal/file.h<br>
> @@ -12,6 +12,7 @@<br>
> #include <sys/types.h><br>
> <br>
> #include <libcamera/class.h><br>
> +#include <libcamera/scoped_file_descriptor.h><br>
> #include <libcamera/span.h><br>
> <br>
> namespace libcamera {<br>
> @@ -40,7 +41,7 @@ public:<br>
> bool exists() const;<br>
> <br>
> bool open(OpenMode mode);<br>
> - bool isOpen() const { return fd_ != -1; }<br>
> + bool isOpen() const { return fd_.isValid(); }<br>
> OpenMode openMode() const { return mode_; }<br>
> void close();<br>
> <br>
> @@ -65,7 +66,7 @@ private:<br>
> void unmapAll();<br>
> <br>
> std::string name_;<br>
> - int fd_;<br>
> + ScopedFD fd_;<br>
> OpenMode mode_;<br>
> <br>
> int error_;<br>
> diff --git a/src/libcamera/file.cpp b/src/libcamera/file.cpp<br>
> index bce2b613..4db9a8d3 100644<br>
> --- a/src/libcamera/file.cpp<br>
> +++ b/src/libcamera/file.cpp<br>
> @@ -73,7 +73,7 @@ LOG_DEFINE_CATEGORY(File)<br>
> * before performing I/O operations.<br>
> */<br>
> File::File(const std::string &name)<br>
> - : name_(name), fd_(-1), mode_(NotOpen), error_(0)<br>
> + : name_(name), mode_(NotOpen), error_(0)<br>
> {<br>
> }<br>
> <br>
> @@ -84,7 +84,7 @@ File::File(const std::string &name)<br>
> * setFileName().<br>
> */<br>
> File::File()<br>
> - : fd_(-1), mode_(NotOpen), error_(0)<br>
> + : mode_(NotOpen), error_(0)<br>
> {<br>
> }<br>
> <br>
> @@ -167,12 +167,13 @@ bool File::open(File::OpenMode mode)<br>
> if (mode & WriteOnly)<br>
> flags |= O_CREAT;<br>
> <br>
> - fd_ = ::open(name_.c_str(), flags, 0666);<br>
> - if (fd_ < 0) {<br>
> + int fd = ::open(name_.c_str(), flags, 0666);<br>
> + if (fd < 0) {<br>
> error_ = -errno;<br>
> return false;<br>
> }<br>
> <br>
> + fd_ = ScopedFD(fd);<br>
> mode_ = mode;<br>
> error_ = 0;<br>
> return true;<br>
> @@ -199,11 +200,10 @@ bool File::open(File::OpenMode mode)<br>
> */<br>
> void File::close()<br>
> {<br>
> - if (fd_ == -1)<br>
> + if (!fd_.isValid())<br>
> return;<br>
> <br>
> - ::close(fd_);<br>
> - fd_ = -1;<br>
> + fd_.reset();<br>
> mode_ = NotOpen;<br>
> }<br>
> <br>
> @@ -233,7 +233,7 @@ ssize_t File::size() const<br>
> return -EINVAL;<br>
> <br>
> struct stat st;<br>
> - int ret = fstat(fd_, &st);<br>
> + int ret = fstat(fd_.get(), &st);<br>
> if (ret < 0)<br>
> return -errno;<br>
> <br>
> @@ -252,7 +252,7 @@ off_t File::pos() const<br>
> if (!isOpen())<br>
> return 0;<br>
> <br>
> - return lseek(fd_, 0, SEEK_CUR);<br>
> + return lseek(fd_.get(), 0, SEEK_CUR);<br>
> }<br>
> <br>
> /**<br>
> @@ -266,7 +266,7 @@ off_t File::seek(off_t pos)<br>
> if (!isOpen())<br>
> return -EINVAL;<br>
> <br>
> - off_t ret = lseek(fd_, pos, SEEK_SET);<br>
> + off_t ret = lseek(fd_.get(), pos, SEEK_SET);<br>
> if (ret < 0)<br>
> return -errno;<br>
> <br>
> @@ -298,7 +298,7 @@ ssize_t File::read(const Span<uint8_t> &data)<br>
> <br>
> /* Retry in case of interrupted system calls. */<br>
> while (readBytes < data.size()) {<br>
> - ret = ::read(fd_, data.data() + readBytes,<br>
> + ret = ::read(fd_.get(), data.data() + readBytes,<br>
> data.size() - readBytes);<br>
> if (ret <= 0)<br>
> break;<br>
> @@ -335,7 +335,7 @@ ssize_t File::write(const Span<const uint8_t> &data)<br>
> <br>
> /* Retry in case of interrupted system calls. */<br>
> while (writtenBytes < data.size()) {<br>
> - ssize_t ret = ::write(fd_, data.data() + writtenBytes,<br>
> + ssize_t ret = ::write(fd_.get(), data.data() + writtenBytes,<br>
> data.size() - writtenBytes);<br>
> if (ret <= 0)<br>
> break;<br>
> @@ -398,7 +398,7 @@ Span<uint8_t> File::map(off_t offset, ssize_t size, enum File::MapFlag flags)<br>
> if (flags & MapPrivate)<br>
> prot |= PROT_WRITE;<br>
> <br>
> - void *map = mmap(NULL, size, prot, mmapFlags, fd_, offset);<br>
> + void *map = mmap(NULL, size, prot, mmapFlags, fd_.get(), offset);<br>
> if (map == MAP_FAILED) {<br>
> error_ = -errno;<br>
> return {};<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>