Re: [yavta PATCH] Return proper error code if STREAMON fails

[Tuukka Toivonen]

Hi,

Thanks for your feedback.

On Saturday, December 12, 2015 17:40:07 Laurent Pinchart wrote:
> I wonder if there's really a point calling video_free_buffers() in the
> error case. The function will return an error causing the caller to
> close the device, which will free the buffers. There are other
> locations in yavta after buffers are allocated where the buffers are not
> freed in the error path. What would you think of just replacing the
> goto done statements by a return ret ?

I'm fine with that change. It will also simplify the patch.
I'll submit a new version of the patch.

- Tuukka

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [yavta PATCH] Return proper error code if STREAMON fails

[Laurent Pinchart]

Hi Tuukka,

Thank you for the patch.

On Monday 30 November 2015 16:46:17 Tuukka Toivonen wrote:
> Fix the bug causing success to be returned even if VIDIOC_STREAMON
> failed. Also check returned error from VIDIOC_STREAMOFF.
> 
> Signed-off-by: Tuukka Toivonen 
> ---
>  yavta.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/yavta.c b/yavta.c
> index b627725..3ad1c97 100644
> --- a/yavta.c
> +++ b/yavta.c
> @@ -1623,7 +1623,7 @@ static int video_do_capture(struct device *dev,
> unsigned int nframes,
>   unsigned int i;
>   double bps;
>   double fps;
> - int ret;
> + int ret, ret2;
> 
>   /* Start streaming. */
>   ret = video_enable(dev, 1);
> @@ -1708,7 +1708,9 @@ static int video_do_capture(struct device *dev,
> unsigned int nframes,
>   }
> 
>   /* Stop streaming. */
> - video_enable(dev, 0);
> + ret = video_enable(dev, 0);
> + if (ret < 0)
> + goto done;
> 
>   if (nframes == 0) {
>   printf("No frames captured.\n");
> @@ -1732,7 +1734,11 @@ static int video_do_capture(struct device *dev,
> unsigned int nframes,
>   i, ts.tv_sec, ts.tv_nsec/1000, fps, bps);
> 
>  done:
> - return video_free_buffers(dev);
> + ret2 = video_free_buffers(dev);

I wonder if there's really a point calling video_free_buffers() in the error 
case. The function will return an error causing the caller to close the 
device, which will free the buffers. There are other locations in yavta after 
buffers are allocated where the buffers are not freed in the error path. What 
would you think of just replacing the goto done statements by a return ret ?

> + if (ret >= 0)
> + ret = ret2;
> +
> + return ret;
>  }
> 
>  #define V4L_BUFFERS_DEFAULT  8

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[yavta PATCH] Return proper error code if STREAMON fails

[Tuukka Toivonen]

Fix the bug causing success to be returned even if VIDIOC_STREAMON
failed. Also check returned error from VIDIOC_STREAMOFF.

Signed-off-by: Tuukka Toivonen 
---
 yavta.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/yavta.c b/yavta.c
index b627725..3ad1c97 100644
--- a/yavta.c
+++ b/yavta.c
@@ -1623,7 +1623,7 @@ static int video_do_capture(struct device *dev, 
unsigned int nframes,
unsigned int i;
double bps;
double fps;
-   int ret;
+   int ret, ret2;
 
/* Start streaming. */
ret = video_enable(dev, 1);
@@ -1708,7 +1708,9 @@ static int video_do_capture(struct device *dev, 
unsigned int nframes,
}
 
/* Stop streaming. */
-   video_enable(dev, 0);
+   ret = video_enable(dev, 0);
+   if (ret < 0)
+   goto done;
 
if (nframes == 0) {
printf("No frames captured.\n");
@@ -1732,7 +1734,11 @@ static int video_do_capture(struct device *dev, 
unsigned int nframes,
i, ts.tv_sec, ts.tv_nsec/1000, fps, bps);
 
 done:
-   return video_free_buffers(dev);
+   ret2 = video_free_buffers(dev);
+   if (ret >= 0)
+   ret = ret2;
+
+   return ret;
 }
 
 #define V4L_BUFFERS_DEFAULT8
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html