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