lkml.org 
[lkml]   [2010]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv3] drivers: isdn: get rid of custom strtoul()
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)

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2010-03-01 15:43    [W:0.022 / U:0.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site