Re: [PATCH 26/45] kstrtox: convert drivers/isdn/

[Tilman Schmidt <t.schmidt@phoenixsoftware.de>]

Am 2010-12-06 21:10 schrieb Alexey Dobriyan:
> On Mon, Dec 06, 2010 at 01:10:45AM +0100, Tilman Schmidt wrote:
>> I like the patch, but why not go all the way?
>>
>> Am 05.12.2010 18:49 schrieb Alexey Dobriyan:
>>> @@ -566,10 +566,10 @@ void gigaset_handle_modem_response(struct cardstate *cs)
>>>  		case RT_ZCAU:
>>>  			event->parameter = -1;
>>>  			if (curarg + 1 < params) {
>>> -				unsigned long type, value;
>>> +				u8 type, value;
>>>  
>>> -				i = strict_strtoul(argv[curarg++], 16, &type);
>>> -				j = strict_strtoul(argv[curarg++], 16, &value);
>>> +				i = kstrtou8(argv[curarg++], 16, &type);
>>> +				j = kstrtou8(argv[curarg++], 16, &value);
>>>  
>>>  				if (i == 0 && type < 256 &&
>>>  				    j == 0 && value < 256)
>>
>> Once type and value are u8, the checks for < 256 are unnecessary.
> 
> OK.
> 
>>> @@ -583,7 +583,7 @@ void gigaset_handle_modem_response(struct cardstate *cs)
>>>  				unsigned long res;
>>>  				int rc;
>>>  
>>> -				rc = strict_strtoul(argv[curarg++], 10, &res);
>>> +				rc = kstrtoul(argv[curarg++], 10, &res);
>>>  				if (rc == 0)
>>>  					event->parameter = res;
>>>  			}
>>
>> The new kstrtoul() promises not to touch the result field in the event
>> of a conversion error, so &event->parameter can be passed directly to
>> it, getting rid of the variables rc and res and the if statement.
> 
> What should be done in case of error?

The same as now: leave event->parameter alone. It's preset to -1
which will later be interpreted as "erroneous or missing value".

> Compiler will warn if kstrto*() result is unused:
> 
> 	kstrtoul(argv[curarg++], 10, &event->parameter);

That's annoying of course. Perhaps silence the warning with some
innocuous pro forma activity like emitting a message:

	if (kstrtoul(argv[curarg++], 10, &event->parameter))
		dev_warn(cs->dev, "bad number");

Or rearrange the code to assign -1 only after an error is
detected, like so:

@@ -578,15 +578,9 @@ void gigaset_handle_modem_response(struct cardstate *cs)
                                curarg = params - 1;
                        break;
                case RT_NUMBER:
-                       event->parameter = -1;
-                       if (curarg < params) {
-                               unsigned long res;
-                               int rc;
-
-                               rc = strict_strtoul(argv[curarg++], 10, &res);
-                               if (rc == 0)
-                                       event->parameter = res;
-                       }
+                       if (curarg >= params ||
+                           kstrtoul(argv[curarg], 10, &event->parameter))
+                               event->parameter = -1;
                        gig_dbg(DEBUG_EVENT, "parameter==%d", event->parameter);
                        break;
                }

Thanks,
Tilman

-- 
Tilman Schmidt
Phoenix Software GmbH
Bonn, Germany