Re: [libvirt] [PATCH 2/3] Move virMacAddrXXX functions to src/util/virmacaddr.[ch]

On 01/27/2012 10:37 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Move the virMacAddrXXX functions out of util.[ch] and into a > new dedicate file virmacaddr.[ch] > --- > src/Makefile.am | 1 + > src/conf/capabilities.h | 2 +- > src/conf/network_conf.h | 2 +- > src/libvirt_private.syms | 11 ++- > src/util/util.c | 99 --------------------------------- > src/util/util.h | 13 ---- > src/util/virmacaddr.c | 128 +++++++++++++++++++++++++++++++++++++++++++ > src/util/virmacaddr.h | 41 ++++++++++++++ > src/util/virnetdev.c | 1 + > src/util/virnetdevmacvlan.c | 2 +- > 10 files changed, 181 insertions(+), 119 deletions(-) More insertions than deletions, but probably due to copyright headers :) > +++ b/src/util/virmacaddr.c > @@ -0,0 +1,128 @@ > +/* > + * virmacaddr.c: MAC address handling > + * > + * Copyright (C) 2012 Red Hat, Inc. When doing code motion, are we required to carry over the copyright years from the earlier location of the code?

> + > +/* Compare two MAC addresses, ignoring differences in case, > + * as well as leading zeros. > + */ > +int > +virMacAddrCompare (const char *p, const char *q) As long as you are moving things, would you like to fix spacing before '('? > +{ > + unsigned char c, d; > + do { > + while (*p == '0' && c_isxdigit (p[1])) > + ++p; > + while (*q == '0' && c_isxdigit (q[1])) and here too? > + ++q; > + c = c_tolower (*p); > + d = c_tolower (*q); and here

> + > + result = strtoul(str, &end_ptr, 16); > + > + if ((end_ptr - str) < 1 || 2 < (end_ptr - str) || > + (errno != 0) || > + (0xFF < result)) That last check is impossible. Yes, I know it's just code motion, and the dead code was present in the original as well, but since we already did a length bound of at most two hex characters, we know that the result did not exceed 0xff. Why are we even bothering with strtoul for a two-character conversion? This seems like it is more efficient when open-coded. But in the interest of pure code motion, I'd do any cleanups as a separate patch.