[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 6 Jul 2016 21:05:26 -0700
From: Stephen Hemminger <stephen@...workplumber.org>
To: Eli Cohen <eli@...lanox.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] Add support for configuring Infiniband GUIDs
On Tue, 5 Jul 2016 10:53:38 -0500
Eli Cohen <eli@...lanox.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
Powered by blists - more mailing lists