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

[Laine Stump]

On 02/10/2011 04:09 PM, Eric Blake wrote:
> 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...)


Done.


>>       <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.


Consistency is good. I made them all caps.


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


Strange - the "c" version is in /usr/share/dict/words, but not in the 
dictionary.


>> +++ 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 $()


All good points. I've switched to your version. (this exercise makes me 
realize how many shell-isms are just wired into my subconscious; for 
example, I'm so used to the file list for grep coming from something 
more complex than a simple glob, that I *always* pipe the filelist 
through xargs to grep even when it's severe overkill, and have never 
noticed sort -u)


> 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).

My thinking was that sed -i is already used elsewhere in the %post 
script on the same file (and that usage doesn't even make a backup in 
case the operation somehow fails, but maybe that's overkill too), so it 
must be good enough for this case too.


>> +             "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

Yes. It was late, and the script ran properly (only because it didn't 
encounter an error), so I didn't notice.

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

Thanks for the (as usual) thorough review!

v3 coming up.