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

[Daniel P. Berrange]

On Fri, Jan 27, 2012 at 10:52:08AM -0700, Eric Blake wrote:
> On 01/27/2012 10:37 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > 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?

Yeah actually I backdated it to 2006

> > +
> > +/* 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

Have done

> > +
> > +        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.

Interesting questions :-)



Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|