[yavta PATCH] Return proper error code if STREAMON fails

[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 <tuukka.toivonen@intel.com>
---
 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_DEFAULT	8
-- 
1.9.1

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 <tuukka.toivonen@intel.com>
> ---
>  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

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

[yavta PATCH v2] Return proper error code if STREAMON fails

[Tuukka Toivonen]

Return the error code if video_enable() and VIDIOC_STREAMON
fails.

Signed-off-by: Tuukka Toivonen <tuukka.toivonen@intel.com>
---
 yavta.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/yavta.c b/yavta.c
index b627725..3d80d3c 100644
--- a/yavta.c
+++ b/yavta.c
@@ -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)
+		return ret;
 
 	if (nframes == 0) {
 		printf("No frames captured.\n");
-- 
1.9.1

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

[Laurent Pinchart]

Hi Tuukka,

On Monday 14 December 2015 09:49:57 Tuukka Toivonen wrote:
> Return the error code if video_enable() and VIDIOC_STREAMON
> fails.
> 
> Signed-off-by: Tuukka Toivonen <tuukka.toivonen@intel.com>

Applied to my tree and pushed, thank you for the patch.

> ---
>  yavta.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/yavta.c b/yavta.c
> index b627725..3d80d3c 100644
> --- a/yavta.c
> +++ b/yavta.c
> @@ -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)
> +		return ret;
> 
>  	if (nframes == 0) {
>  		printf("No frames captured.\n");

-- 
Regards,

Laurent Pinchart