[libvirt] [PATCH] conf: move 'generated' member from virMacAddr to virDomainNetDef

[Daniel P. Berrangé]

On Mon, Feb 19, 2018 at 07:23:23AM +0100, Michal Privoznik wrote:
> On 02/16/2018 09:04 PM, Laine Stump wrote:
> > Commit 7e62c4cd26d (first appearing in libvirt-3.9.0 as a resolution
> > to rhbz #1343919) added a "generated" attribute to virMacAddr that was
> > set whenever a mac address was auto-generated by libvirt. This
> > knowledge was used in a single place - when trying to match a NetDef
> > from the domain to delete with user-provided XML. Since the XML parser
> > always auto-generates a MAC address for NetDefs when none is provided,
> > it was previously impossible to make a search where the MAC address
> > wasn't significant, but the addition of the "generated" attribute made
> > it possible for the search function to ignore auto-generated MACs.
> > 
> > This implementation had a problem though - it was adding a field to a
> > "low level" struct - virMacAddr - which is used in other places with
> > the assumption that it contains exactly a 6 byte MAC address and
> > nothing else. In particular, virNWFilterSnoopEthHdr uses virMacAddr as
> > part of the definition of an ethernet packet header, whose layout must
> > of course match an actual ethernet packet. Adding the extra bools into
> > virNWFilterSnoopEthHdr caused the nwfilter driver's "IP discovery via
> > DHCP packet snooping" functionality to mysteriously stop working.
> > 
> > In order to fix that behavior, and prevent potential future similar
> > odd behavior, this patch moves the "generated" member out of
> > virMacAddr (so that it is again really just a MAC address) and into
> > virDomainNetDef, and sets it only when virDomainNetGenerateMAC() is
> > called from virDomainNetDefParseXML() (which is the only time we care
> > about it).
> > 
> > Resolves: https://bugzilla.redhat.com/1529338
> > 
> > (It should also be applied to any maintenance branch that applies
> > commit 7e62c4cd26 and friends to resolve
> > https://bugzilla.redhat.com/1343919)
> > 
> > Signed-off-by: Laine Stump <laine at laine.org>
> > ---
> >  src/conf/domain_conf.c    | 3 ++-
> >  src/conf/domain_conf.h    | 1 +
> >  src/util/virmacaddr.c     | 5 -----
> >  src/util/virmacaddr.h     | 2 --
> >  tests/bhyveargv2xmlmock.c | 1 -
> >  5 files changed, 3 insertions(+), 9 deletions(-)
> 
> What I am missing here is a comment to _virMacAddr struct saying that
> the structure cannot change because of NWFilter code.

I might be nice to put a compile time assert in nwfilter code

   assert(sizef(struct foobar) == 42);

to validate it matches the ethernet packet size.

> ACK with that changed.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|