Re: [libvirt] [PATCH] conf: forbid use of multicast mac addresses

On 03/19/2012 11:32 AM, Laine Stump wrote: > A few times libvirt users manually setting mac addresses have > complained of networking failure that ends up being due to a multicast > mac address being used for a guest interface. This patch prevents that > by loggin an error and failing if a multicast mac address is s/loggin/logging/ > encountered in each of the three following cases: > > 1) domain xml <interface> mac address. > 2) network xml bridge mac address. > 3) network xml dhcp/host mac address. > > There are several other places where a mac address cna be input that s/cna/can/ > aren't controlled in this manner because failure to do so has no > consequences (e.g., if the address will be used to search through > existing interfaces for a match). > > The RNG has been updated to add multiMacAddr and uniMacAddr along with > the existing macAddr, and macAddr was switched to uniMacAddr where > appropriate. > --- > docs/schemas/basictypes.rng | 17 ++++++++++++++++- > docs/schemas/domaincommon.rng | 6 +++--- > docs/schemas/network.rng | 4 ++-- > src/conf/domain_conf.c | 8 +++++++- > src/conf/network_conf.c | 33 ++++++++++++++++++++++++--------- > src/libvirt_private.syms | 2 ++ > src/util/virmacaddr.c | 13 +++++++++++++ > src/util/virmacaddr.h | 3 ++- Is there any way to add a test of an expected error, where we prove that a multicast mac fails to validate? But since the existing tests use unicast addrs, and those still validate, you've at least tested for no regressions on sane usage. > + <!-- The lowest bit of the 1st byte is the "multicast" bit. a --> > + <!-- uniMacAddr requires that bit to be 0, and a multiMacAddr --> > + <!-- requires it to be 1. Plain macAddr will accept either. --> > + <!-- Currently there is no use of multiMacAddr in libvirt, it --> > + <!-- is included here for documentation/comparison purposes. --> > + <define name="uniMacAddr"> > + <data type="string"> > + <param name="pattern">[a-fA-F0-9][02468aAcCeE](:[a-fA-F0-9]{2}){5}</param> > + </data> > + </define> > + <define name="multiMacAddr"> > + <data type="string"> > + <param name="pattern">[a-fA-F0-9][13579bBdDfF](:[a-fA-F0-9]{2}){5}</param> > + </data> > + </define> > <define name="macAddr"> > <data type="string"> > - <param name="pattern">([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2}</param> > + <param name="pattern">[a-fA-F0-9]{2}(:[a-fA-F0-9]{2}){5}</param> Changed to match the earlier patterns. Nice patterns. ACK. If you do figure out how to add a negative test for expected validation failure, I'm okay if you submit it as a separate followup patch.