[PATCH 1/2] v4l-helpers: Don't close the fd in {}_s_fd

[PATCH 1/2] v4l-helpers: Don't close the fd in {}_s_fd

[Ezequiel Garcia]

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.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 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)
-- 
2.18.0.rc2

[PATCH 2/2] v4l-helpers: Fix EXPBUF queue type

[Ezequiel Garcia]

v4l_queue_export_bufs uses the v4l_fd type when calling
EXPBUF ioctl. However, this doesn't work on mem2mem
where there are one capture queue and one output queue
associated to the device.

The current code calls v4l_queue_export_bufs with the
wrong type, failing as:

fail: v4l2-test-buffers.cpp(544): q_.export_bufs(node)
test VIDIOC_EXPBUF: FAIL

Fix this by using the queue type instead.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 utils/common/v4l-helpers.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h
index 83d8d7d9c073..d6866f04e23a 100644
--- a/utils/common/v4l-helpers.h
+++ b/utils/common/v4l-helpers.h
@@ -1633,7 +1633,7 @@ static inline int v4l_queue_export_bufs(struct v4l_fd *f, struct v4l_queue *q,
 	unsigned b, p;
 	int ret = 0;
 
-	expbuf.type = exp_type ? : f->type;
+	expbuf.type = exp_type ? : q->type;
 	expbuf.flags = O_RDWR;
 	memset(expbuf.reserved, 0, sizeof(expbuf.reserved));
 	for (b = 0; b < v4l_queue_g_buffers(q); b++) {
-- 
2.18.0.rc2

Re: [PATCH 1/2] v4l-helpers: Don't close the fd in {}_s_fd

[Hans Verkuil]

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 <ezequiel@collabora.com>
> ---
>  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)
> 

Re: [PATCH 2/2] v4l-helpers: Fix EXPBUF queue type

[Hans Verkuil]

On 06/28/2018 09:25 PM, Ezequiel Garcia wrote:
> v4l_queue_export_bufs uses the v4l_fd type when calling
> EXPBUF ioctl. However, this doesn't work on mem2mem
> where there are one capture queue and one output queue
> associated to the device.
> 
> The current code calls v4l_queue_export_bufs with the
> wrong type, failing as:
> 
> fail: v4l2-test-buffers.cpp(544): q_.export_bufs(node)
> test VIDIOC_EXPBUF: FAIL
> 
> Fix this by using the queue type instead.

I changed this by requiring that the exp_type is provided by the caller.

Thanks for reporting this!

Regards,

	Hans

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  utils/common/v4l-helpers.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/common/v4l-helpers.h b/utils/common/v4l-helpers.h
> index 83d8d7d9c073..d6866f04e23a 100644
> --- a/utils/common/v4l-helpers.h
> +++ b/utils/common/v4l-helpers.h
> @@ -1633,7 +1633,7 @@ static inline int v4l_queue_export_bufs(struct v4l_fd *f, struct v4l_queue *q,
>  	unsigned b, p;
>  	int ret = 0;
>  
> -	expbuf.type = exp_type ? : f->type;
> +	expbuf.type = exp_type ? : q->type;
>  	expbuf.flags = O_RDWR;
>  	memset(expbuf.reserved, 0, sizeof(expbuf.reserved));
>  	for (b = 0; b < v4l_queue_g_buffers(q); b++) {
> 

Re: [PATCH 1/2] v4l-helpers: Don't close the fd in {}_s_fd

[Ezequiel Garcia]

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

[Ezequiel Garcia]

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