diff mbox series

[FFmpeg-devel,2/9] lavf/v4l2: do not use a context variable unnecessarily

Message ID 20211125150500.25040-2-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/9] lavd/jack: increase buffer size for snprintf()
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Anton Khirnov Nov. 25, 2021, 3:04 p.m. UTC
fd is local to the loop iteration, it is better to store it on stack
than modify the context.
---
 libavdevice/v4l2.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Andreas Rheinhardt Nov. 26, 2021, 5:49 p.m. UTC | #1
Anton Khirnov:
> fd is local to the loop iteration, it is better to store it on stack
> than modify the context.
> ---
>  libavdevice/v4l2.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> index b5997fba33..777867db86 100644
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -1033,16 +1033,17 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
>          return ret;
>      }
>      while ((entry = readdir(dir))) {
> +        int fd = -1;
>          char device_name[256];
>  
>          if (!v4l2_is_v4l_dev(entry->d_name))
>              continue;
>  
>          snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
> -        if ((s->fd = device_open(ctx, device_name)) < 0)
> +        if ((fd = device_open(ctx, device_name)) < 0)
>              continue;
>  
> -        if (v4l2_ioctl(s->fd, VIDIOC_QUERYCAP, &cap) < 0) {
> +        if (v4l2_ioctl(fd, VIDIOC_QUERYCAP, &cap) < 0) {
>              ret = AVERROR(errno);
>              av_log(ctx, AV_LOG_ERROR, "ioctl(VIDIOC_QUERYCAP): %s\n", av_err2str(ret));
>              goto fail;
> @@ -1064,8 +1065,7 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
>                                            &device_list->nb_devices, device)) < 0)
>              goto fail;
>  
> -        v4l2_close(s->fd);
> -        s->fd = -1;
> +        v4l2_close(fd);
>          continue;
>  
>        fail:
> @@ -1074,9 +1074,8 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
>              av_freep(&device->device_description);
>              av_freep(&device);
>          }
> -        if (s->fd >= 0)
> -            v4l2_close(s->fd);
> -        s->fd = -1;
> +        if (fd >= 0)

This checks is unnecessary: One only goes to fail after the device has
been successfully opened.

> +            v4l2_close(fd);
>          break;
>      }
>      closedir(dir);
> 

1. Commit title is wrong: This is lavd.
2. Given that there is no equivalent of avformat_alloc_output_context2()
one has to use avformat_open_input() to use avdevice_list_devices().
This means that it is possible for read_header to have been called
before v4l2_get_device_list() and in this case this commit fixes a file
descriptor leak; it also avoids closing an invalid (-1) file descriptor
v4l2_read_close(). This should be mentioned in the commit message; in
fact, this is the main advantage of this commit.
(I have not checked what would have happened had I actually used this
context with the invalid file descriptor afterwards to read data.)

- Andreas
diff mbox series

Patch

diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index b5997fba33..777867db86 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -1033,16 +1033,17 @@  static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
         return ret;
     }
     while ((entry = readdir(dir))) {
+        int fd = -1;
         char device_name[256];
 
         if (!v4l2_is_v4l_dev(entry->d_name))
             continue;
 
         snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
-        if ((s->fd = device_open(ctx, device_name)) < 0)
+        if ((fd = device_open(ctx, device_name)) < 0)
             continue;
 
-        if (v4l2_ioctl(s->fd, VIDIOC_QUERYCAP, &cap) < 0) {
+        if (v4l2_ioctl(fd, VIDIOC_QUERYCAP, &cap) < 0) {
             ret = AVERROR(errno);
             av_log(ctx, AV_LOG_ERROR, "ioctl(VIDIOC_QUERYCAP): %s\n", av_err2str(ret));
             goto fail;
@@ -1064,8 +1065,7 @@  static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
                                           &device_list->nb_devices, device)) < 0)
             goto fail;
 
-        v4l2_close(s->fd);
-        s->fd = -1;
+        v4l2_close(fd);
         continue;
 
       fail:
@@ -1074,9 +1074,8 @@  static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l
             av_freep(&device->device_description);
             av_freep(&device);
         }
-        if (s->fd >= 0)
-            v4l2_close(s->fd);
-        s->fd = -1;
+        if (fd >= 0)
+            v4l2_close(fd);
         break;
     }
     closedir(dir);