[libcamera-devel] [RFC PATCH 10/10] v4l2: V4L2Camera: Return int in getBufferFd()

[Laurent Pinchart]

Hi Hiro,

On Mon, Jun 07, 2021 at 06:22:21PM +0900, Hirokazu Honda wrote:
> On Mon, Jun 7, 2021 at 3:56 AM Laurent Pinchart wrote:
> > On Thu, Apr 15, 2021 at 05:38:43PM +0900, Hirokazu Honda wrote:
> > > V4L2Camera::getBufferFd() returns FileDescriptor. However, the
> > > file descriptor is still owned by V4L2Camera. It should rather
> > > return an integer to represent V4L2Camera doesn't have the
> > > ownership of the file descriptor.
> >
> > The FileDescriptor class doesn't imply ownership, as it's similar to a
> > shared_ptr<>. I'm not against this patch, but why do you think
> > FileDescriptor is bad here ?
>
> IMO, returning FileDescriptor implies that a caller shares the ownership of it.
> It enables a caller to keep it outlive a callee and even share its
> ownership with others.
> What do you think?

Generally speaking, it may be dangerous to handle file descriptors as
integers, due not only to the risk of leakage, but also to the risk of
use-after-free (or rather use-after-close in this case). FileDescriptor
prevents that.

On the other hand, I agree with you that in this case the caller isn't
meant to use the file descriptor in any way that would outlive the
descriptor stored in V4L2Camera. If it was a generic API, I'd prefer
keeping FileDescriptor, but as V4L2Camera and V4L2CameraProxy are
tightly coupled, it really doesn't matter much. I'm OK with the patch if
you think it's better.

> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > >  src/v4l2/v4l2_camera.cpp       | 6 +++---
> > >  src/v4l2/v4l2_camera.h         | 2 +-
> > >  src/v4l2/v4l2_camera_proxy.cpp | 6 +++---
> > >  3 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 97825c71..cccc6749 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -186,16 +186,16 @@ void V4L2Camera::freeBuffers()
> > >       bufferAllocator_->free(stream);
> > >  }
> > >
> > > -FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> > > +int V4L2Camera::getBufferFd(unsigned int index)
> > >  {
> > >       Stream *stream = config_->at(0).stream();
> > >       const std::vector<std::unique_ptr<FrameBuffer>> &buffers =
> > >               bufferAllocator_->buffers(stream);
> > >
> > >       if (buffers.size() <= index)
> > > -             return FileDescriptor();
> > > +             return -1;
> > >
> > > -     return buffers[index]->planes()[0].fd;
> > > +     return buffers[index]->planes()[0].fd.fd();
> > >  }
> > >
> > >  int V4L2Camera::streamOn()
> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > index d2380462..8efe1642 100644
> > > --- a/src/v4l2/v4l2_camera.h
> > > +++ b/src/v4l2/v4l2_camera.h
> > > @@ -53,7 +53,7 @@ public:
> > >
> > >       int allocBuffers(unsigned int count);
> > >       void freeBuffers();
> > > -     FileDescriptor getBufferFd(unsigned int index);
> > > +     int getBufferFd(unsigned int index);
> > >
> > >       int streamOn();
> > >       int streamOff();
> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > index f8bfe595..8f417421 100644
> > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > @@ -112,14 +112,14 @@ void *V4L2CameraProxy::mmap(void *addr, size_t length, int prot, int flags,
> > >               return MAP_FAILED;
> > >       }
> > >
> > > -     FileDescriptor fd = vcam_->getBufferFd(index);
> > > -     if (!fd.isValid()) {
> > > +     int fd = vcam_->getBufferFd(index);
> > > +     if (fd < 0) {
> > >               errno = EINVAL;
> > >               return MAP_FAILED;
> > >       }
> > >
> > >       void *map = V4L2CompatManager::instance()->fops().mmap(addr, length, prot,
> > > -                                                            flags, fd.fd(), 0);
> > > +                                                            flags, fd, 0);
> > >       if (map == MAP_FAILED)
> > >               return map;
> > >

-- 
Regards,

Laurent Pinchart