[PATCH] v4l2-dbg: report fail reason to the user

[PATCH] v4l2-dbg: report fail reason to the user

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

Report the fail reason to the user when writing a register even if
the verbose mode is switched off.

Remove duplicated code ioctl() call which may cause different ioctl()
function call in case of verbose and non verbose if not handled carefully.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 19c0469c02c3 v4l2-apps/util/v4l2-dbg.cpp
--- a/v4l2-apps/util/v4l2-dbg.cpp	Sat Nov 07 15:51:01 2009 -0200
+++ b/v4l2-apps/util/v4l2-dbg.cpp	Sun Nov 08 14:13:52 2009 +0100
@@ -354,13 +354,14 @@
 {
 	int retVal;

-	if (!options[OptVerbose]) return ioctl(fd, request, parm);
 	retVal = ioctl(fd, request, parm);
-	printf("%s: ", name);
-	if (retVal < 0)
-		printf("failed: %s\n", strerror(errno));
-	else
-		printf("ok\n");
+	if (options[OptVerbose]) {
+		printf("%s: ", name);
+		if (retVal < 0)
+			printf("failed: %s\n", strerror(errno));
+		else
+			printf("ok\n");
+	}

 	return retVal;
 }
@@ -586,8 +587,9 @@

 				printf(" set to 0x%llx\n", set_reg.val);
 			} else {
-				printf("Failed to set register 0x%08llx value 0x%llx\n",
-					set_reg.reg, set_reg.val);
+				printf("Failed to set register 0x%08llx value 0x%llx: "
+					"%s\n",
+					set_reg.reg, set_reg.val, strerror(errno));
 			}
 			set_reg.reg++;
 		}

Re: [PATCH] v4l2-dbg: report fail reason to the user

[Hans Verkuil]

On Sunday 08 November 2009 13:32:50 Németh Márton wrote:
> From: Márton Németh <nm127@freemail.hu>
> 
> Report the fail reason to the user when writing a register even if
> the verbose mode is switched off.
> 
> Remove duplicated code ioctl() call which may cause different ioctl()
> function call in case of verbose and non verbose if not handled carefully.
> 
> Signed-off-by: Márton Németh <nm127@freemail.hu>
> ---
> diff -r 19c0469c02c3 v4l2-apps/util/v4l2-dbg.cpp
> --- a/v4l2-apps/util/v4l2-dbg.cpp	Sat Nov 07 15:51:01 2009 -0200
> +++ b/v4l2-apps/util/v4l2-dbg.cpp	Sun Nov 08 14:13:52 2009 +0100
> @@ -354,13 +354,14 @@
>  {
>  	int retVal;
> 
> -	if (!options[OptVerbose]) return ioctl(fd, request, parm);
>  	retVal = ioctl(fd, request, parm);
> -	printf("%s: ", name);
> -	if (retVal < 0)
> -		printf("failed: %s\n", strerror(errno));
> -	else
> -		printf("ok\n");
> +	if (options[OptVerbose]) {
> +		printf("%s: ", name);
> +		if (retVal < 0)
> +			printf("failed: %s\n", strerror(errno));

Strictly speaking if the printf two line back would produce an error, then
errno would now contain the errno from the printf and not from the ioctl.
So errno should be assigned to a local variable right after the ioctl.

Note that this was already a bug in the original code.

Note also that I fail to see any difference in the old vs the new code.

> +		else
> +			printf("ok\n");
> +	}
> 
>  	return retVal;
>  }
> @@ -586,8 +587,9 @@
> 
>  				printf(" set to 0x%llx\n", set_reg.val);
>  			} else {
> -				printf("Failed to set register 0x%08llx value 0x%llx\n",
> -					set_reg.reg, set_reg.val);
> +				printf("Failed to set register 0x%08llx value 0x%llx: "
> +					"%s\n",

Please keep this printf on one line. Yes, you get a checkpatch warning but
I'd rather have a slightly longer line than an ugly split line.

Regards,

	Hans

> +					set_reg.reg, set_reg.val, strerror(errno));
>  			}
>  			set_reg.reg++;
>  		}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH] v4l2-dbg: report fail reason to the user

[Németh Márton]

Hans Verkuil wrote:
> On Sunday 08 November 2009 13:32:50 Németh Márton wrote:
>> From: Márton Németh <nm127@freemail.hu>
>>
>> Report the fail reason to the user when writing a register even if
>> the verbose mode is switched off.
>>
>> Remove duplicated code ioctl() call which may cause different ioctl()
>> function call in case of verbose and non verbose if not handled carefully.
>>
>> Signed-off-by: Márton Németh <nm127@freemail.hu>
>> ---
>> diff -r 19c0469c02c3 v4l2-apps/util/v4l2-dbg.cpp
>> --- a/v4l2-apps/util/v4l2-dbg.cpp	Sat Nov 07 15:51:01 2009 -0200
>> +++ b/v4l2-apps/util/v4l2-dbg.cpp	Sun Nov 08 14:13:52 2009 +0100
>> @@ -354,13 +354,14 @@
>>  {
>>  	int retVal;
>>
>> -	if (!options[OptVerbose]) return ioctl(fd, request, parm);
>>  	retVal = ioctl(fd, request, parm);
>> -	printf("%s: ", name);
>> -	if (retVal < 0)
>> -		printf("failed: %s\n", strerror(errno));
>> -	else
>> -		printf("ok\n");
>> +	if (options[OptVerbose]) {
>> +		printf("%s: ", name);
>> +		if (retVal < 0)
>> +			printf("failed: %s\n", strerror(errno));
> 
> Strictly speaking if the printf two line back would produce an error, then
> errno would now contain the errno from the printf and not from the ioctl.
> So errno should be assigned to a local variable right after the ioctl.
> 
> Note that this was already a bug in the original code.

You are right, this problem was not solved by the new code.

> Note also that I fail to see any difference in the old vs the new code.

In this function I only tried to modify the programming style: only one call to
ioctl(). In the original code the ioctl() call was duplicated which which
results a not-so-well maintainable code: if ever is needed to modify the parameters
of the ioctl() then in the original code two lines would be needed to modify. The
lines are to be modified exactly the same way in that case which may result different
behaviour in case of verbose switched on or off.

> 
>> +		else
>> +			printf("ok\n");
>> +	}
>>
>>  	return retVal;
>>  }
>> @@ -586,8 +587,9 @@
>>
>>  				printf(" set to 0x%llx\n", set_reg.val);
>>  			} else {
>> -				printf("Failed to set register 0x%08llx value 0x%llx\n",
>> -					set_reg.reg, set_reg.val);
>> +				printf("Failed to set register 0x%08llx value 0x%llx: "
>> +					"%s\n",
> 
> Please keep this printf on one line. Yes, you get a checkpatch warning but
> I'd rather have a slightly longer line than an ugly split line.
> 
> Regards,
> 
> 	Hans
> 
>> +					set_reg.reg, set_reg.val, strerror(errno));
>>  			}
>>  			set_reg.reg++;
>>  		}

Re: [PATCH] v4l2-dbg: report fail reason to the user

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

Report the fail reason to the user when writing a register even if
the verbose mode is switched off.

Remove duplicated code ioctl() call which may cause different ioctl()
function call in case of verbose and non verbose if not handled carefully.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 60f784aa071d v4l2-apps/util/v4l2-dbg.cpp
--- a/v4l2-apps/util/v4l2-dbg.cpp	Wed Nov 11 18:28:53 2009 +0100
+++ b/v4l2-apps/util/v4l2-dbg.cpp	Thu Nov 12 08:21:20 2009 +0100
@@ -353,14 +353,21 @@
 static int doioctl(int fd, int request, void *parm, const char *name)
 {
 	int retVal;
+	int ioctl_errno;

 	if (!options[OptVerbose]) return ioctl(fd, request, parm);
 	retVal = ioctl(fd, request, parm);
-	printf("%s: ", name);
-	if (retVal < 0)
-		printf("failed: %s\n", strerror(errno));
-	else
-		printf("ok\n");
+	if (options[OptVerbose]) {
+		/* Save errno because printf() may modify it */
+		ioctl_errno = errno;
+		printf("%s: ", name);
+		if (retVal < 0)
+			printf("failed: %s\n", strerror(errno));
+		else
+			printf("ok\n");
+		/* Restore errno for caller's use */
+		errno = ioctl_errno;
+	}

 	return retVal;
 }
@@ -586,8 +593,8 @@

 				printf(" set to 0x%llx\n", set_reg.val);
 			} else {
-				printf("Failed to set register 0x%08llx value 0x%llx\n",
-					set_reg.reg, set_reg.val);
+				printf("Failed to set register 0x%08llx value 0x%llx: %s\n",
+					set_reg.reg, set_reg.val, strerror(errno));
 			}
 			set_reg.reg++;
 		}

Re: [PATCH] v4l2-dbg: report fail reason to the user

[Hans Verkuil]

On Thursday 12 November 2009 08:23:41 Németh Márton wrote:
> From: Márton Németh <nm127@freemail.hu>
> 
> Report the fail reason to the user when writing a register even if
> the verbose mode is switched off.
> 
> Remove duplicated code ioctl() call which may cause different ioctl()
> function call in case of verbose and non verbose if not handled carefully.
> 
> Signed-off-by: Márton Németh <nm127@freemail.hu>
> ---
> diff -r 60f784aa071d v4l2-apps/util/v4l2-dbg.cpp
> --- a/v4l2-apps/util/v4l2-dbg.cpp	Wed Nov 11 18:28:53 2009 +0100
> +++ b/v4l2-apps/util/v4l2-dbg.cpp	Thu Nov 12 08:21:20 2009 +0100
> @@ -353,14 +353,21 @@
>  static int doioctl(int fd, int request, void *parm, const char *name)
>  {
>  	int retVal;
> +	int ioctl_errno;
> 
>  	if (!options[OptVerbose]) return ioctl(fd, request, parm);
>  	retVal = ioctl(fd, request, parm);
> -	printf("%s: ", name);
> -	if (retVal < 0)
> -		printf("failed: %s\n", strerror(errno));
> -	else
> -		printf("ok\n");
> +	if (options[OptVerbose]) {
> +		/* Save errno because printf() may modify it */
> +		ioctl_errno = errno;
> +		printf("%s: ", name);
> +		if (retVal < 0)
> +			printf("failed: %s\n", strerror(errno));
> +		else
> +			printf("ok\n");

I'm an idiot for not realizing this when I did the first review, but this
can be done without making a copy of errno. Just do this:

	if (options[OptVerbose]) {
		if (retVal < 0)
			printf("%s: failed: %s\n", name, strerror(errno));
		else
			printf("%s: ok\n", name);

Much simpler :-)

Can you change this and post again? Then I'll add it to my pending pull
request.

Thanks,

	Hans

> +		/* Restore errno for caller's use */
> +		errno = ioctl_errno;
> +	}
> 
>  	return retVal;
>  }
> @@ -586,8 +593,8 @@
> 
>  				printf(" set to 0x%llx\n", set_reg.val);
>  			} else {
> -				printf("Failed to set register 0x%08llx value 0x%llx\n",
> -					set_reg.reg, set_reg.val);
> +				printf("Failed to set register 0x%08llx value 0x%llx: %s\n",
> +					set_reg.reg, set_reg.val, strerror(errno));
>  			}
>  			set_reg.reg++;
>  		}
> 



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH] v4l2-dbg: report fail reason to the user

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

Report the fail reason to the user when writing a register even if
the verbose mode is switched off.

Remove duplicated code ioctl() call which may cause different ioctl()
function call in case of verbose and non verbose if not handled carefully.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 60f784aa071d v4l2-apps/util/v4l2-dbg.cpp
--- a/v4l2-apps/util/v4l2-dbg.cpp	Wed Nov 11 18:28:53 2009 +0100
+++ b/v4l2-apps/util/v4l2-dbg.cpp	Thu Nov 12 09:15:17 2009 +0100
@@ -354,13 +354,14 @@
 {
 	int retVal;

-	if (!options[OptVerbose]) return ioctl(fd, request, parm);
 	retVal = ioctl(fd, request, parm);
-	printf("%s: ", name);
-	if (retVal < 0)
-		printf("failed: %s\n", strerror(errno));
-	else
-		printf("ok\n");
+	if (options[OptVerbose]) {
+		printf("%s: ", name);
+		if (retVal < 0)
+			printf("failed: %s\n", strerror(errno));
+		else
+			printf("ok\n");
+	}

 	return retVal;
 }
@@ -586,8 +587,8 @@

 				printf(" set to 0x%llx\n", set_reg.val);
 			} else {
-				printf("Failed to set register 0x%08llx value 0x%llx\n",
-					set_reg.reg, set_reg.val);
+				printf("Failed to set register 0x%08llx value 0x%llx: %s\n",
+					set_reg.reg, set_reg.val, strerror(errno));
 			}
 			set_reg.reg++;
 		}