[libvirt PATCH v2 0/3] network: Drop UUID handling for default network
Changes from [v1]: * ensure the updated network configuration is written back to disk whenever a UUID has been generated. [v1] https://www.redhat.com/archives/libvir-list/2020-November/msg00785.html Andrea Bolognani (3): conf: Write network config to disk after generating UUID network: Drop UUID handling for default network spec: Drop UUID handling for default network libvirt.spec.in | 4 ---- src/conf/virnetworkobj.c | 17 ++++++++++++++++- src/network/meson.build | 32 +++++++------------------------- 3 files changed, 23 insertions(+), 30 deletions(-) -- 2.26.2
While we generally expect libvirt objects to be defined using the appropriate APIs, there are cases where it's reasonable for an external entity, usually a package manager, to drop a valid configuration file under /etc/libvirt and have libvirt take over from there: notably, this is exactly how the default network is handled. For the most part, whether the configuration is saved back to disk after being parsed by libvirt doesn't matter, because we'll end up with the same values anyway, but an obvious exception to this is data that gets randomly generated when not present, namely MAC address and UUID. Historically, both were handled by our build system, but commit a47ae7c004e9 moved handling of the former inside libvirt proper; this commit extends such behavior to the latter as well. Proper error handling for the virNetworkSaveConfig() call, which was missing until now, is introduced in the process. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/virnetworkobj.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 1b29d7cc64..1e167c7874 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1001,6 +1001,7 @@ virNetworkLoadConfig(virNetworkObjListPtr nets, char *configFile = NULL, *autostartLink = NULL; virNetworkDefPtr def = NULL; virNetworkObjPtr obj; + bool saveConfig = false; int autostart; if ((configFile = virNetworkConfigFile(configDir, name)) == NULL) @@ -1029,7 +1030,10 @@ virNetworkLoadConfig(virNetworkObjListPtr nets, case VIR_NETWORK_FORWARD_OPEN: if (!def->mac_specified) { virNetworkSetBridgeMacAddr(def); - virNetworkSaveConfig(configDir, def, xmlopt); + /* We just generated a new MAC address, and we need to persist + * the configuration to disk to avoid the network getting a + * different one the next time the daemon is started */ + saveConfig = true; } break; @@ -1049,6 +1053,17 @@ virNetworkLoadConfig(virNetworkObjListPtr nets, goto error; } + /* The network didn't have a UUID so we generated a new one, and + * we need to persist the configuration to disk to avoid the network + * getting a different one the next time the daemon is started */ + if (!def->uuid_specified) + saveConfig = true; + + if (saveConfig && + virNetworkSaveConfig(configDir, def, xmlopt) < 0) { + goto error; + } + if (!(obj = virNetworkObjAssignDef(nets, def, 0))) goto error; -- 2.26.2
We are generating a fresh UUID and storing it in the XML for the default network, but this is unnecessary because the network driver will automatically generate one if it's missing from the XML; the fact that we only do this if the uuidgen command happens to be available on the build machine is further proof that we can safely skip this step. This patch is best viewed with 'git show -w'. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/network/meson.build | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/src/network/meson.build b/src/network/meson.build index 13dd2c26b2..3ec598c3f9 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -84,31 +84,13 @@ if conf.has('WITH_NETWORK') runstatedir / 'libvirt' / 'network', ] - uuidgen_prog = find_program('uuidgen', required: false) - - if uuidgen_prog.found() - uuid = run_command(uuidgen_prog).stdout().strip() - - configure_file( - input: 'default.xml.in', - output: '@BASENAME@', - command: [ - 'sed', '-e', 's|</name>|</name>\\n <uuid>@0@</uuid>|'.format(uuid), - '@INPUT@', - ], - capture: true, - install: true, - install_dir: confdir / 'qemu' / 'networks', - ) - else - configure_file( - input: 'default.xml.in', - output: '@BASENAME@', - copy: true, - install: true, - install_dir: confdir / 'qemu' / 'networks', - ) - endif + configure_file( + input: 'default.xml.in', + output: '@BASENAME@', + copy: true, + install: true, + install_dir: confdir / 'qemu' / 'networks', + ) meson.add_install_script( meson_python_prog.path(), python3_prog.path(), meson_install_symlink_prog.path(), -- 2.26.2
We're no longer generating a UUID during installation, so we clearly don't need to strip it afterwards; and since the network driver is perfectly capable of generating a UUID if necessary, we don't need to do that at %post time either. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Neal Gompa <ngompa13@gmail.com> --- libvirt.spec.in | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index de76878bc6..880051b6f8 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1240,8 +1240,6 @@ cp -a $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/nwfilter/*.xml \ # libvirt saves these files with mode 600 chmod 600 $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/nwfilter/*.xml -# Strip auto-generated UUID - we need it generated per-install -sed -i -e "/<uuid>/d" $RPM_BUILD_ROOT%{_datadir}/libvirt/networks/default.xml %if ! %{with_qemu} rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/libvirtd_qemu.aug rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug @@ -1422,9 +1420,7 @@ if test $1 -eq 1 && test ! -f %{_sysconfdir}/libvirt/qemu/networks/default.xml ; ;; esac - UUID=`/usr/bin/uuidgen` sed -e "s/${orig_sub}/${sub}/g" \ - -e "s,</name>,</name>\n <uuid>$UUID</uuid>," \ < %{_datadir}/libvirt/networks/default.xml \ > %{_sysconfdir}/libvirt/qemu/networks/default.xml ln -s ../default.xml %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml -- 2.26.2
On 11/19/20 10:36 AM, Andrea Bolognani wrote:
Changes from [v1]:
* ensure the updated network configuration is written back to disk whenever a UUID has been generated.
[v1] https://www.redhat.com/archives/libvir-list/2020-November/msg00785.html
Andrea Bolognani (3): conf: Write network config to disk after generating UUID network: Drop UUID handling for default network spec: Drop UUID handling for default network
libvirt.spec.in | 4 ---- src/conf/virnetworkobj.c | 17 ++++++++++++++++- src/network/meson.build | 32 +++++++------------------------- 3 files changed, 23 insertions(+), 30 deletions(-)
Reviewed-by: Laine Stump <laine@redhat.com> for the series. We should change the other drivers that have uuids so that they also save if the uuid was added during parse. If that sounds boring to you, then I can put it on a list somewhere. I can't guarantee that will mean that it actually gets done though :-)
On Thu, 2020-11-19 at 11:03 -0500, Laine Stump wrote:
On 11/19/20 10:36 AM, Andrea Bolognani wrote:
Andrea Bolognani (3): conf: Write network config to disk after generating UUID network: Drop UUID handling for default network spec: Drop UUID handling for default network
Reviewed-by: Laine Stump <laine@redhat.com>
for the series.
Thanks, pushed now.
We should change the other drivers that have uuids so that they also save if the uuid was added during parse. If that sounds boring to you, then I can put it on a list somewhere. I can't guarantee that will mean that it actually gets done though :-)
I'm going to take a stab at that, but I felt like there was a very strong case for introducing this behavior for networks specifically because 1) we already have it for MAC addresses and 2) we also already do it for nwfilters, the other libvirt object for which we know for sure we're going to be dropping configuration files into /etc/libvirt at install time. While I think extending this behavior to all libvirt objects is the way to go, I can also see that being a more controversial change, so I didn't to tie the two together and risk this never getting merged because of that :) -- Andrea Bolognani / Red Hat / Virtualization
On 11/20/20 5:53 AM, Andrea Bolognani wrote:
On Thu, 2020-11-19 at 11:03 -0500, Laine Stump wrote:
We should change the other drivers that have uuids so that they also save if the uuid was added during parse. If that sounds boring to you, then I can put it on a list somewhere. I can't guarantee that will mean that it actually gets done though :-)
I'm going to take a stab at that, but I felt like there was a very strong case for introducing this behavior for networks specifically because 1) we already have it for MAC addresses and 2) we also already do it for nwfilters, the other libvirt object for which we know for sure we're going to be dropping configuration files into /etc/libvirt at install time.
While I think extending this behavior to all libvirt objects is the way to go, I can also see that being a more controversial change, so I didn't to tie the two together and risk this never getting merged because of that :)
Yup. I was only suggesting it as something to add to the todo list, not something that needed to be done right away.
participants (2)
-
Andrea Bolognani -
Laine Stump