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