netdev - RE: [PATCH] Add support for configuring Infiniband GUIDs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 7 Jul 2016 21:11:45 +0000
From:	Eli Cohen <eli@...lanox.com>
To:	Stephen Hemminger <stephen@...workplumber.org>,
	"david.laight@...lab.com" <david.laight@...lab.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] Add support for configuring Infiniband GUIDs

Hi,

I have just sent a new version of the patch which addresses all the comments I got.
Thanks for reviewing. Please let me know if you have any other comments.

Thanks,
Eli

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@...workplumber.org] 
Sent: Wednesday, July 06, 2016 11:05 PM
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