From: Stephen Hemminger
> Sent: 07 July 2016 05:05
> On Tue, 5 Jul 2016 10:53:38 -0500
> Eli Cohen <e...@mellanox.com> wrote:
>
> >
> > +static int extract_guid(__u64 *guid, char *arg)
> > +{
> > + __u64 ret;
> > + int g[8];
> > + int err;
> > +
> > + err = sscanf(arg, "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> > + g, g + 1, g + 2, g + 3, g + 4, g + 5, g + 6, g + 7);
> > + if (err != 8)
> > + return -1;
> > +
> > + ret = ((__u64)(g[0]) << 56) |
> > + ((__u64)(g[1]) << 48) |
> > + ((__u64)(g[2]) << 40) |
> > + ((__u64)(g[3]) << 32) |
> > + ((__u64)(g[4]) << 24) |
> > + ((__u64)(g[5]) << 16) |
> > + ((__u64)(g[6]) << 8) |
> > + ((__u64)(g[7]));
> > + *guid = ret;
> > +
> > + return 0;
> > +}
>
> I would like several things changed here.
> 1. put this in generic (ie lib/utils.c) so that other places
> can use it. And rename it match other arg parsing code (ie get_guid)
> 2. need range checking for each piece the string, and each hex piece must be
> unsigned int
> suprised gcc format checks didn't bust you on this. why not %hhx as
> format specifier
>
> 3. arg should be const char *
> 4. local variable err is really unnecessary
> 5. local variable ret is unnecessary, you could just assign to *guid
I'd suggest not using sscanf, but using the kernel equivalent of strtoul()
(or even just looking for [0-9a-fA-F] directly).
sscanf() can bite you in all sorts of unexpected ways.
David
