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@xxxxxxxxxx>
> >
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list