Re: [libvirt] [PATCHv2 2/2] Give each virtual network bridge its own fixed MAC address

On 02/10/2011 02:57 AM, Laine Stump wrote: > The solution is to create a dummy tap interface with a MAC guaranteed > to be lower than any guest interface's MAC, and attach that tap to the > bridge as soon as it's created. Since all guest MAC addresses start > with 0xFE, we can just generate a MAC with the standard "0x52, 0x54, > 0" prefix, and it's guaranteed to always win (physical interfaces are > never connected to these bridges, so we don't need to worry about > competing numerically with them). > > +++ b/docs/formatnetwork.html.in > @@ -105,12 +105,15 @@ > <h3><a name="elementsAddress">Addressing</a></h3> > > <p> > - The final set of elements define the IPv4 address range available, > - and optionally enable DHCP sevices. > + The final set of elements define the addresses (IPv4 and/or > + IPv6, as well as MAC) to be assigned to the bridge device > + associated with the virtual network, and optionally enable DHCP > + sevices. s/sevices/services/ (pre-existing, but as long as you're touching that sentence...)

> <dl> > +<dt><code>mac</code></dt> > +<dd>The<code>address</code> attribute defines a MAC > + (hardware) address formatted as 6 groups of 2-digit > + hexadecimal numbers, the groups separated by colons > + (eg,<code>"52:54:00:1C:DA:2F"</code>). This MAC address is > + assigned to the bridge device when it is created. Generally > + it is best to not specify a mac address when creating a Hmm, for consistency, I'm thinking s/mac/MAC/g > + network - in this case, if a defined mac address is needed for > + proper operation, libvirt will automatically generate a random > + mac address and save it in the config. Allowing libvirt to (3 instances); but feel free to disregard that if you think it results in too many all-caps words.

> + generate the MAC address will assure that it is compatible > + with the idiosyncracies of the platform where libvirt is s/idiosyncracies/idiosyncrasies/

> +++ b/libvirt.spec.in > @@ -741,6 +741,44 @@ then > > %{_sysconfdir}/libvirt/qemu/networks/default.xml > ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml > fi > + > +# All newly defined networks will have a mac address for the bridge > +# auto-generated, but networks already existing at the time of upgrade > +# will not. We need to go through all the network configs, look for > +# those that don't have a mac address, and add one. > + > +network_files=`(cd %{_localstatedir}/lib/libvirt/network; \ > + ls *.xml | xargs grep -L "mac address"; \ > + cd %{_sysconfdir}/libvirt/qemu/networks; \ > + ls *.xml | xargs grep -L "mac address") \ > + | sort | uniq` This leaks a message to stderr if globbing fails: ls: cannot access *.xml: No such file or directory You want to use&& rather than ; after cd (in case for some weird reason cd fails, you don't want to be globbing in the wrong location). sort | uniq wastes a process. For that matter, do we really expect the glob to result in so many networks that we exceed ARG_MAX limitations, or can we ditch the xargs process? `` is yucky - let's use $()

network_files=$( (cd %{_localstatedir}/lib/libvirt/network&& \ grep -L "mac address" *.xml; \ cd %{_sysconfdir}/libvirt/qemu/networks&& \ grep -L "mac address" *.xml) 2>/dev/null \ | sort -u) > + > +for file in $network_files > +do > + # each file exists in either the config or state directory (or both) and > + # does not have a mac address specified in either. We add the same mac > + # address to both files (or just one, if the other isn't there) > + > + mac4=`printf '%X' $(($RANDOM % 256))` > + mac5=`printf '%X' $(($RANDOM % 256))` > + mac6=`printf '%X' $(($RANDOM % 256))` > + for dir in %{_localstatedir}/lib/libvirt/network %{_sysconfdir}/libvirt/qemu/networks > + do > + if test -f $dir/$file > + then > + sed -i.orig -e \ Is sed -i atomic? I know perl -i is not (that is, perl moves the file to a temporary, the puts output into the original, but there exists a window of time where the original file name refers to an incomplete file). Are we better off skipping -i, and doing sed into a secondary file, and atomically renaming that file over the original if all went well? On the other hand, then you have to start worrying about file permissions being accidentally changed. http://www.pixelbeat.org/docs/unix_file_replacement.html Or do we just give up on the atomicity requirement, and call this method good enough (that is, you are unlikely to be upgrading rpm at the same time that libvirt is trying to read (or worse modify) one of these .xml files).

> + "s|\(<bridge.*$\)|\0\n<mac address='52:54:00:$mac4:$mac5:$mac6'/>|" \ > + $dir/$file > + if ! test $? Won't work. $? is always non-empty, which means test $? is always true, which means ! test $? will always take the else branch. You meant: if test $? != 0

The code looked fine, but I think it would be wise to see a v3 with the spec file cleanups and spelling fixes.