drivers: isdn: get rid of custom strtoul()
in [Kernel]
|
Prev: [patch 09/32] [PATCH] smp: rename and add lowcore defines
Next: introduce sys_membarrier(): process-wide memory barrier (v9)
From: Tilman Schmidt on 24 Feb 2010 04:00 -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 2010-02-23 20:21 schrieb Andy Shevchenko: > --- 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_TRANSCMD, "string: %s", p); This conflicts (trivially) with: [PATCH 3/4] gigaset: reduce syslog clutter which I submitted the day before yesterday and which has not yet been merged into *-next. > -/* > - * Get integer from char-pointer > - */ > -static int isdn_gethex(char *p) > -{ > - int v = 0; > - int c; > - > - gig_dbg(DEBUG_TRANSCMD, "string: %s", p); Idem. > /* 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; > + int rc; > > if (s[-1] != ';') > return 0; /* no CID separator */ > - cid = isdn_getnum(s); > - if (cid < 0) > + rc = strict_strtoul(s, 10, &cid); strict_strtoul() expects unsigned long* as its third argument, but cid is an int. Consequently, this generates sparse warnings: drivers/isdn/gigaset/ev-layer.c:433:30: warning: incorrect type in argument 3 (different type sizes) drivers/isdn/gigaset/ev-layer.c:433:30: expected unsigned long *<noident> drivers/isdn/gigaset/ev-layer.c:433:30: got int *<noident> and compiler warnings: drivers/isdn/gigaset/ev-layer.c: In function 'cid_of_response': drivers/isdn/gigaset/ev-layer.c:433: warning: passing argument 3 of 'strict_strtoul' from incompatible pointer type include/linux/kernel.h:176: note: expected 'long unsigned int *' but argument is of type 'int *' > @@ -648,9 +602,11 @@ 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) > + int ri = strict_strtoul(argv[curarg], 16, &i); > + int rj = strict_strtoul(argv[curarg+1], 16, &j); Idem. > @@ -660,12 +616,14 @@ void gigaset_handle_modem_response(struct cardstate *cs) > 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); Idem. Thanks, Tilman - -- Tilman Schmidt E-Mail: tilman(a)imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.12 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkuE6ZMACgkQQ3+did9BuFtLUQCfcPv6XMsFCmSJ+BfU4W42F1k4 tI0AnjsOK0rGkIF3Gnx4S4z3bzMriCw3 =xfZT -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tilman Schmidt on 1 Mar 2010 09:50 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(a)imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
From: Tilman Schmidt on 6 Mar 2010 19:00
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(a)gmail.com> > Cc: Hansjoerg Lipp <hjlipp(a)web.de> > Cc: Tilman Schmidt <tilman(a)imap.cc> Acked-by: Tilman Schmidt <tilman(a)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(a)imap.cc Bonn, Germany Diese Nachricht besteht zu 100% aus wiederverwerteten Bits. Ungeöffnet mindestens haltbar bis: (siehe Rückseite) |