Re: [PATCH 1/2] v4l-helpers: Don't close the fd in {}_s_fd
Hey Hans, On Fri, 2018-06-29 at 09:03 +0200, Hans Verkuil wrote: > On 06/28/2018 09:25 PM, Ezequiel Garcia wrote: > > When creating a second node via copy or assignment: > > > > node2 = node > > > > The node being assigned to, i.e. node2, obtains the fd. > > This causes a later call to node2.media_open to close() > > the fd, thus unintendenly closing the original node fd, > > via the call path (e.g. for media devices): > > > > node2.media_open > > v4l_media_open > > v4l_media_s_fd > > > > Similar call paths apply for other device types. > > Fix this by removing the close in xxx_s_fd. > > I fixed this in a different way by overloading the assignment > operator > and calling dup(fd). That solves this as well. > This patch is also needed to prevent the compliance tool from unintendenly closing a descriptor. diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h index 27683a3d286d..45ed997379a1 100644 --- a/utils/common/v4l-helpers.h +++ b/utils/common/v4l-helpers.h @@ -118,7 +118,11 @@ static inline int v4l_wrap_open(struct v4l_fd *f, const char *file, int oflag, . static inline int v4l_wrap_close(struct v4l_fd *f) { - return close(f->fd); + int ret; + + ret = close(f->fd); + f->fd = -1; + return ret; } static inline ssize_t v4l_wrap_read(struct v4l_fd *f, void *buffer, size_t n) Regards, Eze
Re: [PATCH 1/2] v4l-helpers: Don't close the fd in {}_s_fd
On Fri, 2018-06-29 at 09:03 +0200, Hans Verkuil wrote: > On 06/28/2018 09:25 PM, Ezequiel Garcia wrote: > > When creating a second node via copy or assignment: > > > > node2 = node > > > > The node being assigned to, i.e. node2, obtains the fd. > > This causes a later call to node2.media_open to close() > > the fd, thus unintendenly closing the original node fd, > > via the call path (e.g. for media devices): > > > > node2.media_open > > v4l_media_open > > v4l_media_s_fd > > > > Similar call paths apply for other device types. > > Fix this by removing the close in xxx_s_fd. > > I fixed this in a different way by overloading the assignment > operator > and calling dup(fd). That solves this as well. > Yes, but I am now seeing another EBADF error in the compliance run. close(3)= 0 openat(AT_FDCWD, "/dev/video2", O_RDWR) = 3 close(3)= 0 ioctl(3, VIDIOC_QUERYCAP, 0x7ffe54788794) = -1 EBADF close(3)= -1 EBADF Let me see if I can dig it.
Re: [PATCH 1/2] v4l-helpers: Don't close the fd in {}_s_fd
On 06/28/2018 09:25 PM, Ezequiel Garcia wrote: > When creating a second node via copy or assignment: > > node2 = node > > The node being assigned to, i.e. node2, obtains the fd. > This causes a later call to node2.media_open to close() > the fd, thus unintendenly closing the original node fd, > via the call path (e.g. for media devices): > > node2.media_open > v4l_media_open > v4l_media_s_fd > > Similar call paths apply for other device types. > Fix this by removing the close in xxx_s_fd. I fixed this in a different way by overloading the assignment operator and calling dup(fd). That solves this as well. Regards, Hans > > Signed-off-by: Ezequiel Garcia > --- > utils/common/v4l-helpers.h | 9 - > 1 file changed, 9 deletions(-) > > diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h > index c37b72712126..83d8d7d9c073 100644 > --- a/utils/common/v4l-helpers.h > +++ b/utils/common/v4l-helpers.h > @@ -444,9 +444,6 @@ static inline int v4l_s_fd(struct v4l_fd *f, int fd, > const char *devname, bool d > struct v4l2_queryctrl qc; > struct v4l2_selection sel; > > - if (f->fd >= 0) > - f->close(f); > - > f->fd = fd; > f->direct = direct; > if (fd < 0) > @@ -492,9 +489,6 @@ static inline int v4l_open(struct v4l_fd *f, const char > *devname, bool non_block > > static inline int v4l_subdev_s_fd(struct v4l_fd *f, int fd, const char > *devname) > { > - if (f->fd >= 0) > - f->close(f); > - > f->fd = fd; > f->direct = false; > if (fd < 0) > @@ -525,9 +519,6 @@ static inline int v4l_subdev_open(struct v4l_fd *f, const > char *devname, bool no > > static inline int v4l_media_s_fd(struct v4l_fd *f, int fd, const char > *devname) > { > - if (f->fd >= 0) > - f->close(f); > - > f->fd = fd; > f->direct = false; > if (fd < 0) >