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

[Hirokazu Honda]

Hi Laurent,

On Mon, Jun 28, 2021 at 12:03 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.
>

I would like to keep my idea as the current getBufferFd usage intends
to not share the ownership.
If the usage needs to be changed, I think it will be better to use
FileDescriptor at that time.

Thanks,
-Hiro
> > > 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