sscanf("-1", "%d", &i) fails, returns 0

sscanf("-1", "%d", &i) fails, returns 0

[Douglas Gilbert]

In lk 2.5.46-bk3 the expression in the subject line
fails to write into "i" and returns 0. Drop the minus
sign and it works.

Doug Gilbert

[PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Randy.Dunlap]

On Sat, 9 Nov 2002, Douglas Gilbert wrote:

| In lk 2.5.46-bk3 the expression in the subject line
| fails to write into "i" and returns 0. Drop the minus
| sign and it works.

Here's an unobstrusive patch to correct that.
Please apply.

-- 
~Randy



--- ./lib/vsprintf.c%signed	Mon Nov  4 14:30:49 2002
+++ ./lib/vsprintf.c	Fri Nov  8 11:20:03 2002
@@ -517,6 +517,7 @@
 {
 	const char *str = buf;
 	char *next;
+	char *dig;
 	int num = 0;
 	int qualifier;
 	int base;
@@ -638,12 +639,13 @@
 		while (isspace(*str))
 			str++;

-		if (!*str
-                    || (base == 16 && !isxdigit(*str))
-                    || (base == 10 && !isdigit(*str))
-                    || (base == 8 && (!isdigit(*str) || *str > '7'))
-                    || (base == 0 && !isdigit(*str)))
-			break;
+		dig = (*str == '-') ? (str + 1) : str;
+		if (!*dig
+                    || (base == 16 && !isxdigit(*dig))
+                    || (base == 10 && !isdigit(*dig))
+                    || (base == 8 && (!isdigit(*dig) || *dig > '7'))
+                    || (base == 0 && !isdigit(*dig)))
+				break;

 		switch(qualifier) {
 		case 'h':

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Richard B. Johnson]

On Fri, 8 Nov 2002, Randy.Dunlap wrote:

> On Sat, 9 Nov 2002, Douglas Gilbert wrote:
> 
> | In lk 2.5.46-bk3 the expression in the subject line
> | fails to write into "i" and returns 0. Drop the minus
> | sign and it works.
> 
> Here's an unobstrusive patch to correct that.
> Please apply.
> 
> -- 
> ~Randy
> 
> 
> 
> --- ./lib/vsprintf.c%signed	Mon Nov  4 14:30:49 2002
> +++ ./lib/vsprintf.c	Fri Nov  8 11:20:03 2002
> @@ -517,6 +517,7 @@
>  {
>  	const char *str = buf;
>  	char *next;
> +	char *dig;
>  	int num = 0;
>  	int qualifier;
>  	int base;
> @@ -638,12 +639,13 @@
>  		while (isspace(*str))
>  			str++;
> 
> -		if (!*str
> -                    || (base == 16 && !isxdigit(*str))
> -                    || (base == 10 && !isdigit(*str))
> -                    || (base == 8 && (!isdigit(*str) || *str > '7'))
> -                    || (base == 0 && !isdigit(*str)))
> -			break;
> +		dig = (*str == '-') ? (str + 1) : str;
> +		if (!*dig
> +                    || (base == 16 && !isxdigit(*dig))
> +                    || (base == 10 && !isdigit(*dig))
> +                    || (base == 8 && (!isdigit(*dig) || *dig > '7'))
> +                    || (base == 0 && !isdigit(*dig)))
> +				break;
> 
>  		switch(qualifier) {
>  		case 'h':
> 
> -

I was thinking that if anybody ever had to change any of this
stuff, it might be a good idea to do the indirection only once?
All those "splats" over and over again are costly.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
   Bush : The Fourth Reich of America

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Randy.Dunlap]

On Fri, 8 Nov 2002, Richard B. Johnson wrote:

| On Fri, 8 Nov 2002, Randy.Dunlap wrote:
|
| > On Sat, 9 Nov 2002, Douglas Gilbert wrote:
| >
| > | In lk 2.5.46-bk3 the expression in the subject line
| > | fails to write into "i" and returns 0. Drop the minus
| > | sign and it works.
| >
| > Here's an unobstrusive patch to correct that.
| > Please apply.
| >
| > --
| > ~Randy
| > -
|
| I was thinking that if anybody ever had to change any of this
| stuff, it might be a good idea to do the indirection only once?
| All those "splats" over and over again are costly.

Sure, it looks cleaner that way, although gcc has already put <*dig>
in a local register; i.e., it's not pulled from memory for each test.
Here's a (tested) version that does that.

-- 
~Randy



--- ./lib/vsprintf.c%signed	Mon Nov  4 14:30:49 2002
+++ ./lib/vsprintf.c	Fri Nov  8 12:03:15 2002
@@ -517,6 +517,7 @@
 {
 	const char *str = buf;
 	char *next;
+	char *dig, onedig;
 	int num = 0;
 	int qualifier;
 	int base;
@@ -638,12 +639,14 @@
 		while (isspace(*str))
 			str++;

-		if (!*str
-                    || (base == 16 && !isxdigit(*str))
-                    || (base == 10 && !isdigit(*str))
-                    || (base == 8 && (!isdigit(*str) || *str > '7'))
-                    || (base == 0 && !isdigit(*str)))
-			break;
+		dig = (*str == '-') ? (str + 1) : str;
+		onedig = *dig;
+		if (!onedig
+                    || (base == 16 && !isxdigit(onedig))
+                    || (base == 10 && !isdigit(onedig))
+                    || (base == 8 && (!isdigit(onedig) || onedig > '7'))
+                    || (base == 0 && !isdigit(onedig)))
+				break;

 		switch(qualifier) {
 		case 'h':

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Linus Torvalds]

On Fri, 8 Nov 2002, Randy.Dunlap wrote:
?
> Sure, it looks cleaner that way, although gcc has already put <*dig>
> in a local register; i.e., it's not pulled from memory for each test.
> Here's a (tested) version that does that.

Why do you have that "dig" pointer at all? It's not really used.

Why not just do

	+	char digit;
	...

	+	digit = str;
	+	if (digit == '-')
	+		digit = str[1];


(and maybe it should also test for whether signed stuff is even alloed or 
not, ie maybe the test should be "if (is_sign && digit == '-')" instead)

		Linus

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Richard B. Johnson]

On Fri, 8 Nov 2002, Randy.Dunlap wrote:

> On Fri, 8 Nov 2002, Richard B. Johnson wrote:
> 
> | On Fri, 8 Nov 2002, Randy.Dunlap wrote:
> |
> | > On Sat, 9 Nov 2002, Douglas Gilbert wrote:
> | >
> | > | In lk 2.5.46-bk3 the expression in the subject line
> | > | fails to write into "i" and returns 0. Drop the minus
> | > | sign and it works.
> | >
> | > Here's an unobstrusive patch to correct that.
> | > Please apply.
> | >
> | > --
> | > ~Randy
> | > -
> |
> | I was thinking that if anybody ever had to change any of this
> | stuff, it might be a good idea to do the indirection only once?
> | All those "splats" over and over again are costly.
> 
> Sure, it looks cleaner that way, although gcc has already put <*dig>
> in a local register; i.e., it's not pulled from memory for each test.
> Here's a (tested) version that does that.
> 
> -- 
> ~Randy
> 
> 
> 
> --- ./lib/vsprintf.c%signed	Mon Nov  4 14:30:49 2002
> +++ ./lib/vsprintf.c	Fri Nov  8 12:03:15 2002
> @@ -517,6 +517,7 @@
>  {
>  	const char *str = buf;
>  	char *next;
> +	char *dig, onedig;
>  	int num = 0;
>  	int qualifier;
>  	int base;
> @@ -638,12 +639,14 @@
>  		while (isspace(*str))
>  			str++;
> 
> -		if (!*str
> -                    || (base == 16 && !isxdigit(*str))
> -                    || (base == 10 && !isdigit(*str))
> -                    || (base == 8 && (!isdigit(*str) || *str > '7'))
> -                    || (base == 0 && !isdigit(*str)))
> -			break;
> +		dig = (*str == '-') ? (str + 1) : str;
> +		onedig = *dig;
> +		if (!onedig
> +                    || (base == 16 && !isxdigit(onedig))
> +                    || (base == 10 && !isdigit(onedig))
> +                    || (base == 8 && (!isdigit(onedig) || onedig > '7'))
> +                    || (base == 0 && !isdigit(onedig)))
> +				break;
> 
>  		switch(qualifier) {
>  		case 'h':
> 

I like it much better.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
   Bush : The Fourth Reich of America

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Randy.Dunlap]

On Fri, 8 Nov 2002, Linus Torvalds wrote:

|
| On Fri, 8 Nov 2002, Randy.Dunlap wrote:
| ?
| > Sure, it looks cleaner that way, although gcc has already put <*dig>
| > in a local register; i.e., it's not pulled from memory for each test.
| > Here's a (tested) version that does that.
|
| Why do you have that "dig" pointer at all? It's not really used.
|
| Why not just do
|
| 	+	char digit;
| 	...
|
| 	+	digit = str;
| 	+	if (digit == '-')
| 	+		digit = str[1];
|
|
| (and maybe it should also test for whether signed stuff is even alloed or
| not, ie maybe the test should be "if (is_sign && digit == '-')" instead)

OK, I've cleaned it up as suggested...

-- 
~Randy



--- ./lib/vsprintf.c%signed	Mon Nov  4 14:30:49 2002
+++ ./lib/vsprintf.c	Fri Nov  8 13:54:33 2002
@@ -517,6 +517,7 @@
 {
 	const char *str = buf;
 	char *next;
+	char digit;
 	int num = 0;
 	int qualifier;
 	int base;
@@ -638,12 +639,16 @@
 		while (isspace(*str))
 			str++;

-		if (!*str
-                    || (base == 16 && !isxdigit(*str))
-                    || (base == 10 && !isdigit(*str))
-                    || (base == 8 && (!isdigit(*str) || *str > '7'))
-                    || (base == 0 && !isdigit(*str)))
-			break;
+		digit = *str;
+		if (is_sign && digit == '-')
+			digit = *(str + 1);
+
+		if (!digit
+                    || (base == 16 && !isxdigit(digit))
+                    || (base == 10 && !isdigit(digit))
+                    || (base == 8 && (!isdigit(digit) || digit > '7'))
+                    || (base == 0 && !isdigit(digit)))
+				break;

 		switch(qualifier) {
 		case 'h':

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Henning P. Schmiedehausen]

"Randy.Dunlap" <rddunlap@osdl.org> writes:

>+		digit = *str;
>+		if (is_sign && digit == '-')
>+			digit = *(str + 1);

If signed is not allowed and you get a "-", you're in an error case
again...

	Regards
		Henning

-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen       -- Geschaeftsfuehrer
INTERMETA - Gesellschaft fuer Mehrwertdienste mbH     hps@intermeta.de

Am Schwabachgrund 22  Fon.: 09131 / 50654-0   info@intermeta.de
D-91054 Buckenhof     Fax.: 09131 / 50654-20   

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Randy.Dunlap]

On Sun, 10 Nov 2002, Henning P. Schmiedehausen wrote:

| "Randy.Dunlap" <rddunlap@osdl.org> writes:
|
| >+		digit = *str;
| >+		if (is_sign && digit == '-')
| >+			digit = *(str + 1);
|
| If signed is not allowed and you get a "-", you're in an error case
| again...

Yes, and a 0 value is returned.
IOW, asking for an unsigned number (in the format string)
and getting "-123" does return 0.

What should it do?
This function can't return -EINPUTERROR or -EILSEQ.
(since it's after feature-freeze :)
And the original problem was that a leading '-' sign on a
signed number (!) caused a return of 0.  At least that is fixed.

So now the problem (?) is that a '-' sign on an unsigned number
returns 0.  We can always add a big printk() there that
something is foul.  Other ideas?

-- 
~Randy

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Randy.Dunlap]

On Sun, 10 Nov 2002, Randy.Dunlap wrote:

| On Sun, 10 Nov 2002, Henning P. Schmiedehausen wrote:
|
| | "Randy.Dunlap" <rddunlap@osdl.org> writes:
| |
| | >+		digit = *str;
| | >+		if (is_sign && digit == '-')
| | >+			digit = *(str + 1);
| |
| | If signed is not allowed and you get a "-", you're in an error case
| | again...
|
| Yes, and a 0 value is returned.
| IOW, asking for an unsigned number (in the format string)
| and getting "-123" does return 0.
|
| What should it do?
| This function can't return -EINPUTERROR or -EILSEQ.
| (since it's after feature-freeze :)
| And the original problem was that a leading '-' sign on a
| signed number (!) caused a return of 0.  At least that is fixed.
|
| So now the problem (?) is that a '-' sign on an unsigned number
| returns 0.  We can always add a big printk() there that
| something is foul.  Other ideas?

It's noteworthy that vsscanf() completely gives up on scanning
the rest of the input data at this point.  E.g.:
	count = sscanf (input, "%x %i %i", &level, &next, &other);
with <input> = "-42 -86 -99" gives up on "-42" and returns 0
(as number of items scanned/converted).

-- 
~Randy

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Henning Schmiedehausen]

Hi,

On Mon, 2002-11-11 at 04:05, Randy.Dunlap wrote:
> On Sun, 10 Nov 2002, Henning P. Schmiedehausen wrote:
> 
> | "Randy.Dunlap" <rddunlap@osdl.org> writes:
> |
> | >+		digit = *str;
> | >+		if (is_sign && digit == '-')
> | >+			digit = *(str + 1);
> |
> | If signed is not allowed and you get a "-", you're in an error case
> | again...
> 
> Yes, and a 0 value is returned.
> IOW, asking for an unsigned number (in the format string)
> and getting "-123" does return 0.
> 
> What should it do?

I would model this after user space. (Which does strange things:

--- cut ---
#include <stdio.h>

main()
{
  char *scan = "-100";
  unsigned int foo;
  int bar;

  int res = sscanf(scan, "%ud", &foo);

  printf("%s = %ud = %d\n", scan, foo, res);

  res = sscanf(scan, "%ud", &bar);

  printf("%s = %d = %d\n", scan, bar, res);

}
--- cut ---

% gcc -o xxx xxx.c
./xxx 
-100 = 4294967196d = 1
-100 = -100 = 1

Hm, so I guess, yes, a warning message would be nice IMHO. Returning an
error code would IMHO moot, because noone is checking these codes
anyway.

	Regards
		Henning



> This function can't return -EINPUTERROR or -EILSEQ.
> (since it's after feature-freeze :)
> And the original problem was that a leading '-' sign on a
> signed number (!) caused a return of 0.  At least that is fixed.
> 
> So now the problem (?) is that a '-' sign on an unsigned number
> returns 0.  We can always add a big printk() there that
> something is foul.  Other ideas?


-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen       -- Geschaeftsfuehrer
INTERMETA - Gesellschaft fuer Mehrwertdienste mbH     hps@intermeta.de

Am Schwabachgrund 22  Fon.: 09131 / 50654-0   info@intermeta.de
D-91054 Buckenhof     Fax.: 09131 / 50654-20   

Re: [PATCH] Re: sscanf("-1", "%d", &i) fails, returns 0

[Andreas Schwab]

"Randy.Dunlap" <rddunlap@osdl.org> writes:

|> On Sun, 10 Nov 2002, Henning P. Schmiedehausen wrote:
|> 
|> | "Randy.Dunlap" <rddunlap@osdl.org> writes:
|> |
|> | >+		digit = *str;
|> | >+		if (is_sign && digit == '-')
|> | >+			digit = *(str + 1);
|> |
|> | If signed is not allowed and you get a "-", you're in an error case
|> | again...
|> 
|> Yes, and a 0 value is returned.
|> IOW, asking for an unsigned number (in the format string)
|> and getting "-123" does return 0.

Not in C.  According to the standard scanf is supposed to convert the
value to unsinged and return that.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."