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:step...@networkplumber.org] Sent: Wednesday, July 06, 2016 11:05 PM To: Eli Cohen <e...@mellanox.com> Cc: netdev@vger.kernel.org Subject: Re: [PATCH] Add support for configuring Infiniband GUIDs 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
