Re: [PATCHv3] drivers: isdn: get rid of custom strtoul()

Re: [PATCHv3] drivers: isdn: get rid of custom strtoul()

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 2867 bytes --]

Am 2010-02-26 08:26 schrieb Andy Shevchenko:
> There were two methods isdn_gethex() and isdn_getnum() which are custom
> implementations of strtoul(). Get rid of them in regard to
> strict_strtoul() kernel's function.
> 
> The patch should be applied on top of "[PATCH 3/4] gigaset: reduce syslog
> clutter" [1]

That patch is now in net-next, so that remark isn't needed anymore.

> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
[...]
> @@ -647,24 +601,28 @@ void gigaset_handle_modem_response(struct cardstate *cs)
>  		case RT_ZCAU:
>  			event->parameter = -1;
>  			if (curarg + 1 < params) {
> -				i = isdn_gethex(argv[curarg]);
> -				j = isdn_gethex(argv[curarg + 1]);
> -				if (i >= 0 && i < 256 && j >= 0 && j < 256)
> -					event->parameter = (unsigned) i << 8
> -							   | j;
> -				curarg += 2;
> +				unsigned long hi, lo;
> +				int rh, rl;
> +
> +				rh = strict_strtoul(argv[curarg++], 16, &hi);
> +				rl = strict_strtoul(argv[curarg++], 16, &lo);
> +
> +				if (rh == 0 && hi < 256 && rl == 0 && lo < 256)
> +					event->parameter = (hi << 8) | lo;

If you want to give the two decoded value variables speaking names (as
opposed to the generic "i" and "j") I'd prefer that they correspond to
their actual meaning (which of course you couldn't know). What you
called "hi" is the "cause type", and what you called "lo", the "cause
value". More appropriate variable names would therefore be, for example,
"cautype" and "cauvalue".

Of course that's only cosmetic, but it would still be nice if you could
take it into account if you're doing another version of the patch.

>  			} else
>  				curarg = params - 1;
>  			break;
>  		case RT_NUMBER:
>  		case RT_HEX:
>  			if (curarg < params) {
> -				if (param_type == RT_HEX)
> -					event->parameter =
> -						isdn_gethex(argv[curarg]);
> -				else
> -					event->parameter =
> -						isdn_getnum(argv[curarg]);
> +				int base = (param_type == RT_HEX) ? 16 : 10;
> +				unsigned long res;
> +				int rc;
> +
> +				rc = strict_strtoul(argv[curarg], base, &res);
> +				if (rc == 0)
> +					event->parameter = res;
> +

That's not quite correct. (Sorry for not catching that in the last
review.) The original code would set event->parameter to -1 if the
argv[curarg] string isn't valid. Your code will leave it unchanged
in that case.

As a remedy, you can just set event->parameter = -1 at the beginning
of the case, just as in the case RT_ZCAU above. That will also make
the else clause below unnecessary.

>  				++curarg;
>  			} else
>  				event->parameter = -1;

Thanks,
Tilman

-- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCHv3] drivers: isdn: get rid of custom strtoul()

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 3845 bytes --]

Am 26.02.2010 08:26 schrieb Andy Shevchenko:
> There were two methods isdn_gethex() and isdn_getnum() which are custom
> implementations of strtoul(). Get rid of them in regard to
> strict_strtoul() kernel's function.
> 
> The patch should be applied on top of "[PATCH 3/4] gigaset: reduce syslog
> clutter" [1]
> 
> [1] http://patchwork.kernel.org/patch/81327/
> 
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Hansjoerg Lipp <hjlipp@web.de>
> Cc: Tilman Schmidt <tilman@imap.cc>

Acked-by: Tilman Schmidt <tilman@imap.cc>

Thanks,
Tilman

> ---
>  drivers/isdn/gigaset/ev-layer.c |   82 +++++++++-----------------------------
>  1 files changed, 20 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/ev-layer.c b/drivers/isdn/gigaset/ev-layer.c
> index c8f89b7..b1a334b 100644
> --- a/drivers/isdn/gigaset/ev-layer.c
> +++ b/drivers/isdn/gigaset/ev-layer.c
> @@ -420,64 +420,18 @@ static const struct zsau_resp_t {
>  	{NULL,				ZSAU_UNKNOWN}
>  };
>  
> -/*
> - * Get integer from char-pointer
> - */
> -static int isdn_getnum(char *p)
> -{
> -	int v = -1;
> -
> -	gig_dbg(DEBUG_EVENT, "string: %s", p);
> -
> -	while (*p >= '0' && *p <= '9')
> -		v = ((v < 0) ? 0 : (v * 10)) + (int) ((*p++) - '0');
> -	if (*p)
> -		v = -1; /* invalid Character */
> -	return v;
> -}
> -
> -/*
> - * Get integer from char-pointer
> - */
> -static int isdn_gethex(char *p)
> -{
> -	int v = 0;
> -	int c;
> -
> -	gig_dbg(DEBUG_EVENT, "string: %s", p);
> -
> -	if (!*p)
> -		return -1;
> -
> -	do {
> -		if (v > (INT_MAX - 15) / 16)
> -			return -1;
> -		c = *p;
> -		if (c >= '0' && c <= '9')
> -			c -= '0';
> -		else if (c >= 'a' && c <= 'f')
> -			c -= 'a' - 10;
> -		else if (c >= 'A' && c <= 'F')
> -			c -= 'A' - 10;
> -		else
> -			return -1;
> -		v = v * 16 + c;
> -	} while (*++p);
> -
> -	return v;
> -}
> -
>  /* retrieve CID from parsed response
>   * returns 0 if no CID, -1 if invalid CID, or CID value 1..65535
>   */
>  static int cid_of_response(char *s)
>  {
> -	int cid;
> +	unsigned long cid;
> +	int rc;
>  
>  	if (s[-1] != ';')
>  		return 0;	/* no CID separator */
> -	cid = isdn_getnum(s);
> -	if (cid < 0)
> +	rc = strict_strtoul(s, 10, &cid);
> +	if (rc)
>  		return 0;	/* CID not numeric */
>  	if (cid < 1 || cid > 65535)
>  		return -1;	/* CID out of range */
> @@ -647,24 +601,28 @@ void gigaset_handle_modem_response(struct cardstate *cs)
>  		case RT_ZCAU:
>  			event->parameter = -1;
>  			if (curarg + 1 < params) {
> -				i = isdn_gethex(argv[curarg]);
> -				j = isdn_gethex(argv[curarg + 1]);
> -				if (i >= 0 && i < 256 && j >= 0 && j < 256)
> -					event->parameter = (unsigned) i << 8
> -							   | j;
> -				curarg += 2;
> +				unsigned long hi, lo;
> +				int rh, rl;
> +
> +				rh = strict_strtoul(argv[curarg++], 16, &hi);
> +				rl = strict_strtoul(argv[curarg++], 16, &lo);
> +
> +				if (rh == 0 && hi < 256 && rl == 0 && lo < 256)
> +					event->parameter = (hi << 8) | lo;
>  			} else
>  				curarg = params - 1;
>  			break;
>  		case RT_NUMBER:
>  		case RT_HEX:
>  			if (curarg < params) {
> -				if (param_type == RT_HEX)
> -					event->parameter =
> -						isdn_gethex(argv[curarg]);
> -				else
> -					event->parameter =
> -						isdn_getnum(argv[curarg]);
> +				int base = (param_type == RT_HEX) ? 16 : 10;
> +				unsigned long res;
> +				int rc;
> +
> +				rc = strict_strtoul(argv[curarg], base, &res);
> +				if (rc == 0)
> +					event->parameter = res;
> +
>  				++curarg;
>  			} else
>  				event->parameter = -1;

-- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]