[libvirt] [PATCHv2 0/4] network: fix active status & usage of inactive networks
The original patch series is here: https://www.redhat.com/archives/libvir-list/2014-April/msg00454.html This repost addresses problems pointed out by John Ferlan in his review (and other things that I thought better of during the revisions). All 4 patches are required to address https://bugzilla.redhat.com/show_bug.cgi?id=880483 The reported problem is that an interface using a currently-inactive macvtap/hostdev network can be attached to a domain (addressed by 4/4). In order to make fixing that problem less, well, problematic, the automatic inactivation of all macvtap/hostdev networks any time libvirtd is restart must be addressed, and that is done in 1/4-3/4. Part of fixing the latter problem is changing the network driver to use /var/run for its status XML rather than /var/lib (2/4), which could cause problems during upgrade (also addressed in 2/4) and in a more limited sense downgrade (see the comments in 2/4 for why I decided to not address those problems). Laine Stump (4): network: create statedir during driver initialization network: change location of network state xml files network: set macvtap/hostdev networks active if their state file exists network: centralize check for active network during interface attach src/conf/network_conf.c | 1 + src/lxc/lxc_driver.c | 21 +---- src/lxc/lxc_process.c | 18 +---- src/network/bridge_driver.c | 189 ++++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.c | 18 +---- src/qemu/qemu_hotplug.c | 11 +-- 6 files changed, 166 insertions(+), 92 deletions(-) -- 1.9.0
This directory should be created when the network driver is first started up, not just when a dhcp daemon is run. This hasn't posed a problem in the past, because the directory has always been pre-existing. --- Change from V1: mistakenly went to "cleanup" on error rather than "error" :-P src/network/bridge_driver.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index eb276cd..4ca3de5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -487,6 +487,13 @@ networkStateInitialize(bool privileged, } } + if (virFileMakePath(driverState->stateDir) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + driverState->stateDir); + goto error; + } + /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); @@ -1171,12 +1178,6 @@ networkStartDhcpDaemon(virNetworkDriverStatePtr driver, driverState->pidDir); goto cleanup; } - if (virFileMakePath(driverState->stateDir) < 0) { - virReportSystemError(errno, - _("cannot create directory %s"), - driverState->stateDir); - goto cleanup; - } if (!(pidfile = virPidFileBuildPath(driverState->pidDir, network->def->name))) -- 1.9.0
On 04/17/2014 07:43 AM, Laine Stump wrote:
This directory should be created when the network driver is first started up, not just when a dhcp daemon is run. This hasn't posed a problem in the past, because the directory has always been pre-existing. --- Change from V1: mistakenly went to "cleanup" on error rather than "error" :-P
src/network/bridge_driver.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
ACK John
For some reason these have been stored in /var/lib, although other drivers (e.g. qemu and lxc) store their state files in /var/run. It's much nicer to store state files in /var/run because it is automatically cleared out when the system reboots. We can then use existence of the state file as a convenient indicator of whether or not a particular network is active. Since changing the location of the state files by itself will cause problems in the case of a *live* upgrade from an older libvirt that uses /var/lib (because current status of active networks will be lost), the network driver initialization has been modified to migrate any network state files from /var/lib to /var/run. This will not help those trying to *downgrade*, but in practice this will only be problematic in two cases 1) If there are networks with network-wide bandwidth limits configured *and in use* by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten. 2) If someone does this: 1) upgrade libvirt, 2) downgrade libvirt, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config). --- change from V1: * merged previous 2/5 & 3/5 into a single patch that changes the location and migrates old state files, to avoid potential problems by people trying to use git bisect. * move the files with a direct copy, rather than reading/parsing the XML then formatting/writing it. Note (hopefully) correct use of readdir! * put the migration into a separate static function * don't put oldStateDir in driverState, as we only use it once during initialization. * only attempt migration when running privileged, since the unprivileged state location hasn't changed. src/network/bridge_driver.c | 97 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 91 insertions(+), 6 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4ca3de5..57dfb2d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -41,6 +41,7 @@ #include <sys/wait.h> #include <sys/ioctl.h> #include <net/if.h> +#include <dirent.h> #if HAVE_SYS_SYSCTL_H # include <sys/sysctl.h> #endif @@ -416,6 +417,88 @@ firewalld_dbus_filter_bridge(DBusConnection *connection ATTRIBUTE_UNUSED, } #endif +static int +networkMigrateStateFiles(virNetworkDriverStatePtr driver) +{ + /* Due to a change in location of network state xml beginning in + * libvirt 1.2.4 (from /var/lib/libvirt/network to + * /var/run/libvirt/network), we must check for state files in two + * locations. Anything found in the old location must be written + * to the new location, then erased from the old location. (Note + * that we read/write the file rather than calling rename() + * because the old and new state directories are likely in + * different filesystems). + */ + int ret = -1; + const char *oldStateDir = LOCALSTATEDIR "/lib/libvirt/network"; + DIR *dir; + struct dirent *entry; + char *oldPath = NULL, *newPath = NULL; + char *contents = NULL; + + if (!(dir = opendir(oldStateDir))) { + if (errno == ENOENT) + return 0; + + virReportSystemError(errno, _("failed to open directory '%s'"), + oldStateDir); + return -1; + } + + if (virFileMakePath(driver->stateDir) < 0) { + virReportSystemError(errno, _("cannot create directory %s"), + driver->stateDir); + goto cleanup; + } + + for (;;) { + errno = 0; + entry = readdir(dir); + if (!entry) { + if (errno) { + virReportSystemError(errno, _("failed to read directory '%s'"), + oldStateDir); + goto cleanup; + } + break; + } + + if (entry->d_type != DT_REG || + STREQ(entry->d_name, ".") || + STREQ(entry->d_name, "..")) + continue; + + if (virAsprintf(&oldPath, "%s/%s", + oldStateDir, entry->d_name) < 0) + goto cleanup; + if (virFileReadAll(oldPath, 1024*1024, &contents) < 0) + goto cleanup; + + if (virAsprintf(&newPath, "%s/%s", + driver->stateDir, entry->d_name) < 0) + goto cleanup; + if (virFileWriteStr(newPath, contents, S_IRUSR | S_IWUSR) < 0) { + virReportSystemError(errno, + _("failed to write network status file '%s'"), + newPath); + goto cleanup; + } + + unlink(oldPath); + VIR_FREE(oldPath); + VIR_FREE(newPath); + VIR_FREE(contents); + } + + ret = 0; + cleanup: + closedir(dir); + VIR_FREE(oldPath); + VIR_FREE(newPath); + VIR_FREE(contents); + return ret; +} + /** * networkStateInitialize: * @@ -445,11 +528,6 @@ networkStateInitialize(bool privileged, /* configuration/state paths are one of * ~/.config/libvirt/... (session/unprivileged) * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged). - * - * NB: The qemu driver puts its domain state in /var/run, and I - * think the network driver should have used /var/run too (instead - * of /var/lib), but it's been this way for a long time, and we - * probably shouldn't change it now. */ if (privileged) { if (VIR_STRDUP(driverState->networkConfigDir, @@ -457,7 +535,7 @@ networkStateInitialize(bool privileged, VIR_STRDUP(driverState->networkAutostartDir, SYSCONFDIR "/libvirt/qemu/networks/autostart") < 0 || VIR_STRDUP(driverState->stateDir, - LOCALSTATEDIR "/lib/libvirt/network") < 0 || + LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->pidDir, LOCALSTATEDIR "/run/libvirt/network") < 0 || VIR_STRDUP(driverState->dnsmasqStateDir, @@ -465,6 +543,13 @@ networkStateInitialize(bool privileged, VIR_STRDUP(driverState->radvdStateDir, LOCALSTATEDIR "/lib/libvirt/radvd") < 0) goto error; + + /* migration from old to new location is only applicable for + * privileged mode - unprivileged mode directories haven't + * changed location. + */ + if (networkMigrateStateFiles(driverState) < 0) + goto error; } else { configdir = virGetUserConfigDirectory(); rundir = virGetUserRuntimeDirectory(); -- 1.9.0
On 04/17/2014 07:43 AM, Laine Stump wrote:
For some reason these have been stored in /var/lib, although other drivers (e.g. qemu and lxc) store their state files in /var/run.
It's much nicer to store state files in /var/run because it is automatically cleared out when the system reboots. We can then use existence of the state file as a convenient indicator of whether or not a particular network is active.
Since changing the location of the state files by itself will cause problems in the case of a *live* upgrade from an older libvirt that uses /var/lib (because current status of active networks will be lost), the network driver initialization has been modified to migrate any network state files from /var/lib to /var/run.
This will not help those trying to *downgrade*, but in practice this will only be problematic in two cases
1) If there are networks with network-wide bandwidth limits configured *and in use* by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten.
2) If someone does this: 1) upgrade libvirt, 2) downgrade libvirt, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config). --- change from V1:
* merged previous 2/5 & 3/5 into a single patch that changes the location and migrates old state files, to avoid potential problems by people trying to use git bisect.
* move the files with a direct copy, rather than reading/parsing the XML then formatting/writing it. Note (hopefully) correct use of readdir!
* put the migration into a separate static function
* don't put oldStateDir in driverState, as we only use it once during initialization.
* only attempt migration when running privileged, since the unprivileged state location hasn't changed.
src/network/bridge_driver.c | 97 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 91 insertions(+), 6 deletions(-)
ACK; however... Your call if you want to wait for virDirRead() from: http://www.redhat.com/archives/libvir-list/2014-April/msg00745.html Then refactor your for (;;) loop to use it... I'm "assuming" the files cannot be adjusted during the period when libvirt is "migrating" by any other "legal" :-) means. Just typing "out loud" since your v1 would lock the network object and save the status in the new location. John
On 04/23/2014 09:52 PM, John Ferlan wrote:
On 04/17/2014 07:43 AM, Laine Stump wrote:
For some reason these have been stored in /var/lib, although other drivers (e.g. qemu and lxc) store their state files in /var/run.
It's much nicer to store state files in /var/run because it is automatically cleared out when the system reboots. We can then use existence of the state file as a convenient indicator of whether or not a particular network is active.
Since changing the location of the state files by itself will cause problems in the case of a *live* upgrade from an older libvirt that uses /var/lib (because current status of active networks will be lost), the network driver initialization has been modified to migrate any network state files from /var/lib to /var/run.
This will not help those trying to *downgrade*, but in practice this will only be problematic in two cases
1) If there are networks with network-wide bandwidth limits configured *and in use* by a guest during a downgrade to "old" libvirt. In this case, the class ID's used for that network's tc rules, as well as the currently in-use bandwidth "floor" will be forgotten.
2) If someone does this: 1) upgrade libvirt, 2) downgrade libvirt, 3) modify running state of network (e.g. add a static dhcp host, etc), 4) upgrade. In this case, the modifications to the running network will be lost (but not any persistent changes to the network's config). --- change from V1:
* merged previous 2/5 & 3/5 into a single patch that changes the location and migrates old state files, to avoid potential problems by people trying to use git bisect.
* move the files with a direct copy, rather than reading/parsing the XML then formatting/writing it. Note (hopefully) correct use of readdir!
* put the migration into a separate static function
* don't put oldStateDir in driverState, as we only use it once during initialization.
* only attempt migration when running privileged, since the unprivileged state location hasn't changed.
src/network/bridge_driver.c | 97 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 91 insertions(+), 6 deletions(-)
ACK; however... Your call if you want to wait for virDirRead() from:
http://www.redhat.com/archives/libvir-list/2014-April/msg00745.html
Then refactor your for (;;) loop to use it...
I think I would rather push it as-is, then send a separate patch to switch to virDirRead() (unless ncopa adds that to his series). The reason I'd rather keep it separate is to decrease the number of prerequisite patches in case this one is backported.
I'm "assuming" the files cannot be adjusted during the period when libvirt is "migrating" by any other "legal" :-) means.
No, the network driver is unusable by anyone until networkStateInitialize() returns success.
Just typing "out loud" since your v1 would lock the network object and save the status in the new location.
That was just a by-product of using a higher level method to do the copy (read status into the network list, then write the status back out).
On 04/24/2014 04:30 AM, Laine Stump wrote:
ACK; however... Your call if you want to wait for virDirRead() from:
http://www.redhat.com/archives/libvir-list/2014-April/msg00745.html
Then refactor your for (;;) loop to use it...
I think I would rather push it as-is, then send a separate patch to switch to virDirRead() (unless ncopa adds that to his series). The reason I'd rather keep it separate is to decrease the number of prerequisite patches in case this one is backported.
Based on that thread, it looks like ncopa wasn't planning on doing any more conversions; I've started the work of finishing his series, and will post it later today. I agree with your approach of pushing your patch first without virDirRead, and then saving the conversion to that new patch. At any rate, my conversion to virDirRead adds a syntax check so we won't forget. And in the worst case, if virDirRead goes in first, the series is intentionally divided to provide the new function in one patch, then convert a few files at a time, then add the syntax check last, so that it's easy to backport just the new function and one or two conversions without the syntax check, if needed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
libvirt attempts to determine at startup time which networks are already active, and set their active flags. Previously it has done this by assuming that all networks are inactive, then setting the active flag if the network has a bridge device associated with it and that bridge device exists. This is not useful for macvtap and hostdev based networks, since they do not use a bridge device. Of course the reason that such a check had to be done was that the presence of a status file in the network "stateDir" couldn't be trusted as an indicator of whether or not a network was active. This was due to the network driver mistakenly using /var/lib/libvirt/network to store the status files, rather than /var/run/libvirt/network (similar to what is done by every other libvirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you can be assured that the state file you are seeing isn't just left over from a previous boot of the host. Now that the network driver has been switched to using /var/run/libvirt/network for status, we can also modify it to assume that any network with an existing status file is by definition active - we do this when reading the status file. To fine tune the results, networkFindActiveConfigs() is changed to networkUpdateAllState(), and only sets active = 0 if the conditions for particular network types are *not* met. The result is that during the first run of libvirtd after the host boots, there are no status files, so no networks are active. Any time libvirtd is restarted, any network with a status file will be marked as active (unless the network uses a bridge device and that device for some reason doesn't exist). --- Changes from V1: * rename networkFindActiveConfigs() to networkUpdateAllState() (rather than networkFindInactiveConfigs() * undo the change in order of calling the above function vs. virNetworkReadAllConfigs(), just in case that would cause some undetected regression. * extricate the reading of pidfiles from the switch statement that behaves differently for different types of networks - those networks that don't use dnsmasq/radvd will not have any pidfiles for them anyway, so it becomes a NOP (and if a new network type that *does* use one of those processes is created, it will automatically work correctly here.) src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 69 ++++++++++++++++++++++++++++++++------------- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..69ad929 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, net->floor_sum = floor_sum_val; net->taint = taint; + net->active = 1; /* any network with a state file is by definition active */ cleanup: VIR_FREE(configFile); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 57dfb2d..0c879b9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,38 +328,69 @@ networkBridgeDummyNicName(const char *brname) return nicname; } +/* Update the internal status of all allegedly active networks + * according to external conditions on the host (i.e. anything that + * isn't stored directly in each network's state file). */ static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkUpdateAllState(virNetworkDriverStatePtr driver) { size_t i; for (i = 0; i < driver->networks.count; i++) { virNetworkObjPtr obj = driver->networks.objs[i]; + if (!obj->active) + continue; + virNetworkObjLock(obj); - /* If bridge exists, then mark it active */ - if (obj->def->bridge && - virNetDevExists(obj->def->bridge) == 1) { - obj->active = 1; + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge exists, then mark it active */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + obj->active = 0; + break; - /* Try and read dnsmasq/radvd pids if any */ - if (obj->def->ips && (obj->def->nips > 0)) { - char *radvdpidbase; + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) + obj->active = 0; + break; + } + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + /* so far no extra checks */ + break; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, - &obj->dnsmasqPid, - dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); + case VIR_NETWORK_FORWARD_HOSTDEV: + /* so far no extra checks */ + break; + } - if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) - goto cleanup; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, - &obj->radvdPid, RADVD)); - VIR_FREE(radvdpidbase); - } + /* Try and read dnsmasq/radvd pids of active networks */ + if (obj->active && obj->def->ips && (obj->def->nips > 0)) { + char *radvdpidbase; + + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + obj->def->name, + &obj->dnsmasqPid, + dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + break; + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + radvdpidbase, + &obj->radvdPid, RADVD)); + VIR_FREE(radvdpidbase); } - cleanup: virNetworkObjUnlock(obj); } @@ -591,7 +622,7 @@ networkStateInitialize(bool privileged, driverState->networkAutostartDir) < 0) goto error; - networkFindActiveConfigs(driverState); + networkUpdateAllState(driverState); networkReloadFirewallRules(driverState); networkRefreshDaemons(driverState); -- 1.9.0
On 04/17/2014 07:43 AM, Laine Stump wrote:
libvirt attempts to determine at startup time which networks are already active, and set their active flags. Previously it has done this by assuming that all networks are inactive, then setting the active flag if the network has a bridge device associated with it and that bridge device exists. This is not useful for macvtap and hostdev based networks, since they do not use a bridge device.
Of course the reason that such a check had to be done was that the presence of a status file in the network "stateDir" couldn't be trusted as an indicator of whether or not a network was active. This was due to the network driver mistakenly using /var/lib/libvirt/network to store the status files, rather than /var/run/libvirt/network (similar to what is done by every other libvirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you can be assured that the state file you are seeing isn't just left over from a previous boot of the host.
Now that the network driver has been switched to using /var/run/libvirt/network for status, we can also modify it to assume that any network with an existing status file is by definition active - we do this when reading the status file. To fine tune the results, networkFindActiveConfigs() is changed to networkUpdateAllState(), and only sets active = 0 if the conditions for particular network types are *not* met.
The result is that during the first run of libvirtd after the host boots, there are no status files, so no networks are active. Any time libvirtd is restarted, any network with a status file will be marked as active (unless the network uses a bridge device and that device for some reason doesn't exist). --- Changes from V1:
* rename networkFindActiveConfigs() to networkUpdateAllState() (rather than networkFindInactiveConfigs()
* undo the change in order of calling the above function vs. virNetworkReadAllConfigs(), just in case that would cause some undetected regression.
* extricate the reading of pidfiles from the switch statement that behaves differently for different types of networks - those networks that don't use dnsmasq/radvd will not have any pidfiles for them anyway, so it becomes a NOP (and if a new network type that *does* use one of those processes is created, it will automatically work correctly here.)
src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 69 ++++++++++++++++++++++++++++++++------------- 2 files changed, 51 insertions(+), 19 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..69ad929 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, net->floor_sum = floor_sum_val;
net->taint = taint; + net->active = 1; /* any network with a state file is by definition active */
Typing and thinking - it's a dangerous combination... Since the driver initialization now "moves" files from /lib/ to /pid/ without regard to whether they were active previously in /lib/ - is it possible that other code will erroneously mark something active? The prior code would look through the state files to mark active... This code says, I found a state file thus I must be active. Of course only true for migration case - so hmm.... does the previous patch need a tweak?
cleanup: VIR_FREE(configFile); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 57dfb2d..0c879b9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,38 +328,69 @@ networkBridgeDummyNicName(const char *brname) return nicname; }
+/* Update the internal status of all allegedly active networks + * according to external conditions on the host (i.e. anything that + * isn't stored directly in each network's state file). */ static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkUpdateAllState(virNetworkDriverStatePtr driver) { size_t i;
for (i = 0; i < driver->networks.count; i++) { virNetworkObjPtr obj = driver->networks.objs[i];
+ if (!obj->active) + continue; + virNetworkObjLock(obj);
- /* If bridge exists, then mark it active */ - if (obj->def->bridge && - virNetDevExists(obj->def->bridge) == 1) { - obj->active = 1; + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge exists, then mark it active */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + obj->active = 0; + break;
Comment is still misleading... ACK with that change John
- /* Try and read dnsmasq/radvd pids if any */ - if (obj->def->ips && (obj->def->nips > 0)) { - char *radvdpidbase; + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) + obj->active = 0; + break; + } + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + /* so far no extra checks */ + break;
- ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, - &obj->dnsmasqPid, - dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); + case VIR_NETWORK_FORWARD_HOSTDEV: + /* so far no extra checks */ + break; + }
- if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) - goto cleanup; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, - &obj->radvdPid, RADVD)); - VIR_FREE(radvdpidbase); - } + /* Try and read dnsmasq/radvd pids of active networks */ + if (obj->active && obj->def->ips && (obj->def->nips > 0)) { + char *radvdpidbase; + + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + obj->def->name, + &obj->dnsmasqPid, + dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + break; + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + radvdpidbase, + &obj->radvdPid, RADVD)); + VIR_FREE(radvdpidbase); }
- cleanup: virNetworkObjUnlock(obj); }
@@ -591,7 +622,7 @@ networkStateInitialize(bool privileged, driverState->networkAutostartDir) < 0) goto error;
- networkFindActiveConfigs(driverState); + networkUpdateAllState(driverState); networkReloadFirewallRules(driverState); networkRefreshDaemons(driverState);
[Did you miss Patch 0.5/4? Or just get to the end of the day first?] On 04/23/2014 10:00 PM, John Ferlan wrote:
On 04/17/2014 07:43 AM, Laine Stump wrote:
libvirt attempts to determine at startup time which networks are already active, and set their active flags. Previously it has done this by assuming that all networks are inactive, then setting the active flag if the network has a bridge device associated with it and that bridge device exists. This is not useful for macvtap and hostdev based networks, since they do not use a bridge device.
Of course the reason that such a check had to be done was that the presence of a status file in the network "stateDir" couldn't be trusted as an indicator of whether or not a network was active. This was due to the network driver mistakenly using /var/lib/libvirt/network to store the status files, rather than /var/run/libvirt/network (similar to what is done by every other libvirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you can be assured that the state file you are seeing isn't just left over from a previous boot of the host.
Now that the network driver has been switched to using /var/run/libvirt/network for status, we can also modify it to assume that any network with an existing status file is by definition active - we do this when reading the status file. To fine tune the results, networkFindActiveConfigs() is changed to networkUpdateAllState(), and only sets active = 0 if the conditions for particular network types are *not* met.
The result is that during the first run of libvirtd after the host boots, there are no status files, so no networks are active. Any time libvirtd is restarted, any network with a status file will be marked as active (unless the network uses a bridge device and that device for some reason doesn't exist). --- Changes from V1:
* rename networkFindActiveConfigs() to networkUpdateAllState() (rather than networkFindInactiveConfigs()
* undo the change in order of calling the above function vs. virNetworkReadAllConfigs(), just in case that would cause some undetected regression.
* extricate the reading of pidfiles from the switch statement that behaves differently for different types of networks - those networks that don't use dnsmasq/radvd will not have any pidfiles for them anyway, so it becomes a NOP (and if a new network type that *does* use one of those processes is created, it will automatically work correctly here.)
src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 69 ++++++++++++++++++++++++++++++++------------- 2 files changed, 51 insertions(+), 19 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..69ad929 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, net->floor_sum = floor_sum_val;
net->taint = taint; + net->active = 1; /* any network with a state file is by definition active */ Typing and thinking - it's a dangerous combination... Since the driver initialization now "moves" files from /lib/ to /pid/ without regard to whether they were active previously in /lib/ - is it possible that other code will erroneously mark something active?
The prior code would look through the state files to mark active... This code says, I found a state file thus I must be active. Of course only true for migration case - so hmm.... does the previous patch need a tweak?
Unfortunately there isn't any other place to look to see if the network is active. In the case of a network that uses a bridge device, here is the difference: OLD: assume network is inactive, but mark it active if a bridge device is found, whether state file exists or not. Bridge Device Yes No State File Yes A I No A I NEW: *IFF* state file exists, assume network is active, but mark it inactive if bridge doesn't exist. Bridge Device Yes No State File Yes A I No I I So the one difference is when there is no state file but the bridge device exists. However, the previous run of (and version of) libvirt would have never marked the network Active in the first place if it was unable to successfully write the state file, so if anything we could encounter cases where the state file *existed* when the network should actually be inactive (the opposite of what would cause a problem), and that would only occur if a "new" libvirt is run immediately after rebooting a system that last ran the "old" libvirt prior to reboot (which isn't the way that upgrades normally happen). In the case of networks that *don't* use a bridge device... well, their status was already broken, and that's what we're trying to fix - basically before these patches, those networks were just *always* considered to be inactive when libvirtd started.
cleanup: VIR_FREE(configFile); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 57dfb2d..0c879b9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,38 +328,69 @@ networkBridgeDummyNicName(const char *brname) return nicname; }
+/* Update the internal status of all allegedly active networks + * according to external conditions on the host (i.e. anything that + * isn't stored directly in each network's state file). */ static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkUpdateAllState(virNetworkDriverStatePtr driver) { size_t i;
for (i = 0; i < driver->networks.count; i++) { virNetworkObjPtr obj = driver->networks.objs[i];
+ if (!obj->active) + continue; + virNetworkObjLock(obj);
- /* If bridge exists, then mark it active */ - if (obj->def->bridge && - virNetDevExists(obj->def->bridge) == 1) { - obj->active = 1; + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge exists, then mark it active */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + obj->active = 0; + break; Comment is still misleading...
Yep, fixed that finally. Thanks!
ACK with that change
The check for a network being active during interface attach was being done individually in several places (by both the lxc driver and the qemu driver), but those places were too specific, leading to it *not* being checked when allocating a connection/device from a macvtap or hostdev network. This patch puts a single check in networkAllocateActualDevice(), which is always called before the any network interface is attached to any type of domain. It also removes all the other now-redundant checks from the lxc and qemu drivers. [Note that prior to the previous patches in this series, it would have been very cumbersome to apply this current patch, as macvtap and hostdev networks would be automatically set inactive at each libvirtd restart (unless they were marked as "autostart"). Therefore, those patches should be seen as prerequisites to this patch for any backporting. This comment is a placeholder that I intend to replace with the git commit id's for those patches as soon as I have pushed them.] This fixes: https://bugzilla.redhat.com/show_bug.cgi?id=880483 --- No change from V1. src/lxc/lxc_driver.c | 21 +++------------------ src/lxc/lxc_process.c | 18 ++---------------- src/network/bridge_driver.c | 10 +++++++++- src/qemu/qemu_command.c | 18 ++---------------- src/qemu/qemu_hotplug.c | 11 +---------- 5 files changed, 17 insertions(+), 61 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 826d918..dba2182 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4155,27 +4155,12 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj; - if (!(network = virNetworkLookupByName(conn, - net->data.network.name))) + if (!(network = virNetworkLookupByName(conn, net->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true; /* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 091102b..0aef13a 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -387,27 +387,13 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virNetworkPtr network; char *brname = NULL; bool fail = false; - int active; virErrorPtr errobj; if (!(network = virNetworkLookupByName(conn, def->nets[i]->data.network.name))) goto cleanup; - - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - def->nets[i]->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true; /* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0c879b9..8ea9f95 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3170,7 +3170,8 @@ static int networkDestroy(virNetworkPtr net) if (!virNetworkObjIsActive(network)) { virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("network is not active")); + _("network '%s' is not active"), + network->def->name); goto cleanup; } @@ -3514,6 +3515,13 @@ networkAllocateActualDevice(virDomainDefPtr dom, } netdef = network->def; + if (!virNetworkObjIsActive(network)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("network '%s' is not active"), + netdef->name); + goto error; + } + if (VIR_ALLOC(iface->data.network.actual) < 0) goto error; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8b4a57a..6265c32 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -297,7 +297,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; bool fail = false; virErrorPtr errobj; virNetworkPtr network = virNetworkLookupByName(conn, @@ -305,21 +304,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (!network) return ret; - active = virNetworkIsActive(network); - if (active != 1) { - fail = true; - - if (active == 0) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } - - if (!fail) { - brname = virNetworkGetBridgeName(network); - if (brname == NULL) - fail = true; - } + if (!(brname = virNetworkGetBridgeName(network))) + fail = true; /* Make sure any above failure is preserved */ errobj = virSaveLastError(); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 233b183..ccfb358 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1710,7 +1710,6 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) if (VIR_STRDUP(brname, tmpbr) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - int active; virErrorPtr errobj; virNetworkPtr network; @@ -1720,15 +1719,7 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) net->data.network.name); goto cleanup; } - - active = virNetworkIsActive(network); - if (active == 1) { - brname = virNetworkGetBridgeName(network); - } else if (active == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Network '%s' is not active."), - net->data.network.name); - } + brname = virNetworkGetBridgeName(network); /* Make sure any above failure is preserved */ errobj = virSaveLastError(); -- 1.9.0
On 04/17/2014 07:43 AM, Laine Stump wrote:
The check for a network being active during interface attach was being done individually in several places (by both the lxc driver and the qemu driver), but those places were too specific, leading to it *not* being checked when allocating a connection/device from a macvtap or hostdev network.
This patch puts a single check in networkAllocateActualDevice(), which is always called before the any network interface is attached to any type of domain. It also removes all the other now-redundant checks from the lxc and qemu drivers.
[Note that prior to the previous patches in this series, it would have been very cumbersome to apply this current patch, as macvtap and hostdev networks would be automatically set inactive at each libvirtd restart (unless they were marked as "autostart"). Therefore, those patches should be seen as prerequisites to this patch for any backporting. This comment is a placeholder that I intend to replace with the git commit id's for those patches as soon as I have pushed them.]
This fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=880483 --- No change from V1.
src/lxc/lxc_driver.c | 21 +++------------------ src/lxc/lxc_process.c | 18 ++---------------- src/network/bridge_driver.c | 10 +++++++++- src/qemu/qemu_command.c | 18 ++---------------- src/qemu/qemu_hotplug.c | 11 +---------- 5 files changed, 17 insertions(+), 61 deletions(-)
ACK - still applies. John
Experimentation showed that if virNetworkCreateXML() was called for a network that was already defined, and then the network was subsequently shutdown, the network would continue to be persistent after the shutdown (expected/desired), but the original config would be lost in favor of the transient config sent in with virNetworkCreateXML() (which would then be the new persistent config) (obviously unexpected/not desired). To fix this, virNetworkObjAssignDef() has been changed to 1) properly save/free network->def and network->newDef for all the various combinations of live/active/persistent, including some combinations that were previously considered to be an error but didn't need to be (e.g. setting a "live" config for a network that isn't yet active but soon will be - that was previously considered an error, even though in practice it can be very useful). 2) automatically set the persistent flag whenever a new non-live config is assigned to the network (and clear it when the non-live config is set to NULL). the libvirt network driver no longer directly manipulates network->persistent, but instead relies entirely on virNetworkObjAssignDef() to do the right thing automatically. After this patch, the following sequence will behave as expected: virNetworkDefineXML(X) virNetworkCreateXML(X') (same name but some config different) virNetworkDestroy(X) At the end of these calls, the network config will remain as it was after the initial virNetworkDefine(), whereas previously it would take on the changes given during virNetworkCreateXML(). Another effect of this tighter coupling between a) setting a !live def and b) setting/clearing the "persistent" flag, is that future patches which change the details of network lifecycle management (e.g. upcoming patches to fix detection of "active" networks when libvirtd is restarted) will find it much more difficult to break persistence functionality. --- src/conf/network_conf.c | 78 +++++++++++++++++++++++---------------- src/conf/network_conf.h | 6 +-- src/network/bridge_driver.c | 34 +++++++---------- src/parallels/parallels_network.c | 4 +- src/test/test_driver.c | 5 +-- 5 files changed, 64 insertions(+), 63 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..66945ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets) /* * virNetworkObjAssignDef: * @network: the network object to update - * @def: the new NetworkDef (will be consumed by this function iff successful) + * @def: the new NetworkDef (will be consumed by this function) * @live: is this new def the "live" version, or the "persistent" version * - * Replace the appropriate copy of the given network's NetworkDef + * Replace the appropriate copy of the given network's def or newDef * with def. Use "live" and current state of the network to determine - * which to replace. + * which to replace and what to do with the old defs. When a non-live + * def is set, indicate that the network is now persistent. + * + * NB: a persistent network can be made transient by calling with: + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the + * persistent def to NULL) * * Returns 0 on success, -1 on failure. */ -int +void virNetworkObjAssignDef(virNetworkObjPtr network, virNetworkDefPtr def, bool live) { - if (virNetworkObjIsActive(network)) { - if (live) { + if (live) { + /* before setting new live def, save (into newDef) any + * existing persistent (!live) def to be restored when the + * network is destroyed, unless there is one already saved. + */ + if (network->persistent && !network->newDef) + network->newDef = network->def; + else virNetworkDefFree(network->def); - network->def = def; - } else if (network->persistent) { - /* save current configuration to be restored on network shutdown */ - virNetworkDefFree(network->newDef); + network->def = def; + } else { /* !live */ + virNetworkDefFree(network->newDef); + if (virNetworkObjIsActive(network)) { + /* save new configuration to be restored on network + * shutdown, leaving current live def alone + */ network->newDef = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save persistent config of transient " - "network '%s'"), network->def->name); - return -1; + } else { /* !live and !active */ + if (network->def && !network->persistent) { + /* network isn't (yet) marked active or persistent, + * but already has a "live" def set. This means we are + * currently setting the persistent def as a part of + * the process of starting the network, so we need to + * preserve the "not yet live" def in network->def. + */ + network->newDef = def; + } else { + /* either there is no live def set, or this network + * was already set as persistent, so the proper thing + * is to overwrite network->def. + */ + network->newDef = NULL; + virNetworkDefFree(network->def); + network->def = def; + } } - } else if (!live) { - virNetworkDefFree(network->newDef); - virNetworkDefFree(network->def); - network->newDef = NULL; - network->def = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save live config of inactive " - "network '%s'"), network->def->name); - return -1; + network->persistent = !!def; } - return 0; } /* @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkObjPtr network; if ((network = virNetworkFindByName(nets, def->name))) { - if (virNetworkObjAssignDef(network, def, live) < 0) { - virNetworkObjUnlock(network); - return NULL; - } + virNetworkObjAssignDef(network, def, live); return network; } @@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets, ignore_value(virBitmapSetBit(network->class_id, 2)); network->def = def; - + network->persistent = !live; return network; + error: virNetworkObjUnlock(network); virNetworkObjFree(network); @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, goto error; net->autostart = autostart; - net->persistent = 1; VIR_FREE(configFile); VIR_FREE(autostartLink); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1a864de..90da16f 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefPtr def, bool live); -int virNetworkObjAssignDef(virNetworkObjPtr network, - virNetworkDefPtr def, - bool live); +void virNetworkObjAssignDef(virNetworkObjPtr network, + virNetworkDefPtr def, + bool live); int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live); void virNetworkObjUnsetDefTransient(virNetworkObjPtr network); virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index eb276cd..62d1c25 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, true) < 0) goto cleanup; - /* NB: "live" is false because this transient network hasn't yet - * been started + /* NB: even though this transient network hasn't yet been started, + * we assign the def with live = true in anticipation that it will + * be started momentarily. */ - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + if (!(network = virNetworkAssignDef(&driver->networks, def, true))) goto cleanup; def = NULL; @@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, false) < 0) goto cleanup; - if ((network = virNetworkFindByName(&driver->networks, def->name))) { - network->persistent = 1; - if (virNetworkObjAssignDef(network, def, false) < 0) - goto cleanup; - } else { - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) - goto cleanup; - } - - /* define makes the network persistent - always */ - network->persistent = 1; + if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + goto cleanup; - /* def was asigned */ + /* def was assigned to network object */ freeDef = false; if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) network = NULL; goto cleanup; } - network->persistent = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + /* if network was active already, just undo new persistent + * definition by making it transient. + * XXX - this isn't necessarily the correct thing to do. + */ + virNetworkObjAssignDef(network, NULL, false); goto cleanup; } @@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net) goto cleanup; /* make the network transient */ - network->persistent = 0; network->autostart = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + virNetworkObjAssignDef(network, NULL, false); event = virNetworkEventLifecycleNew(network->def->name, network->def->uuid, diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index cbb44b9..0ebeb7b 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -2,7 +2,7 @@ * parallels_network.c: core privconn functions for managing * Parallels Cloud Server hosts * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2014 Red Hat, Inc. * Copyright (C) 2012 Parallels, Inc. * * This library is free software; you can redistribute it and/or @@ -231,7 +231,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; } net->active = 1; - net->persistent = 1; net->autostart = 1; virNetworkObjUnlock(net); return net; @@ -267,7 +266,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) goto cleanup; } net->active = 1; - net->persistent = 1; net->autostart = 1; virNetworkObjUnlock(net); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 64f3daa..37756e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -784,7 +784,6 @@ testOpenDefault(virConnectPtr conn) goto error; } netobj->active = 1; - netobj->persistent = 1; virNetworkObjUnlock(netobj); if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML))) @@ -1155,7 +1154,6 @@ testParseNetworks(testConnPtr privconn, goto error; } - obj->persistent = 1; obj->active = 1; virNetworkObjUnlock(obj); } @@ -3711,7 +3709,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup; - if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) + if (!(net = virNetworkAssignDef(&privconn->networks, def, true))) goto cleanup; def = NULL; net->active = 1; @@ -3748,7 +3746,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) goto cleanup; def = NULL; - net->persistent = 1; event = virNetworkEventLifecycleNew(net->def->name, net->def->uuid, VIR_NETWORK_EVENT_DEFINED, -- 1.9.0
On 04/23/2014 09:49 AM, Laine Stump wrote:
Experimentation showed that if virNetworkCreateXML() was called for a network that was already defined, and then the network was subsequently shutdown, the network would continue to be persistent after the shutdown (expected/desired), but the original config would be lost in favor of the transient config sent in with virNetworkCreateXML() (which would then be the new persistent config) (obviously unexpected/not desired).
To fix this, virNetworkObjAssignDef() has been changed to
1) properly save/free network->def and network->newDef for all the various combinations of live/active/persistent, including some combinations that were previously considered to be an error but didn't need to be (e.g. setting a "live" config for a network that isn't yet active but soon will be - that was previously considered an error, even though in practice it can be very useful).
2) automatically set the persistent flag whenever a new non-live config is assigned to the network (and clear it when the non-live config is set to NULL). the libvirt network driver no longer directly manipulates network->persistent, but instead relies entirely on virNetworkObjAssignDef() to do the right thing automatically.
After this patch, the following sequence will behave as expected:
virNetworkDefineXML(X) virNetworkCreateXML(X') (same name but some config different) virNetworkDestroy(X)
At the end of these calls, the network config will remain as it was after the initial virNetworkDefine(), whereas previously it would take on the changes given during virNetworkCreateXML().
Another effect of this tighter coupling between a) setting a !live def and b) setting/clearing the "persistent" flag, is that future patches which change the details of network lifecycle management (e.g. upcoming patches to fix detection of "active" networks when libvirtd is restarted) will find it much more difficult to break persistence functionality. --- src/conf/network_conf.c | 78 +++++++++++++++++++++++---------------- src/conf/network_conf.h | 6 +-- src/network/bridge_driver.c | 34 +++++++---------- src/parallels/parallels_network.c | 4 +- src/test/test_driver.c | 5 +-- 5 files changed, 64 insertions(+), 63 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..66945ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets) /* * virNetworkObjAssignDef: * @network: the network object to update - * @def: the new NetworkDef (will be consumed by this function iff successful) + * @def: the new NetworkDef (will be consumed by this function) * @live: is this new def the "live" version, or the "persistent" version * - * Replace the appropriate copy of the given network's NetworkDef + * Replace the appropriate copy of the given network's def or newDef * with def. Use "live" and current state of the network to determine - * which to replace. + * which to replace and what to do with the old defs. When a non-live + * def is set, indicate that the network is now persistent. + * + * NB: a persistent network can be made transient by calling with: + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the + * persistent def to NULL) * * Returns 0 on success, -1 on failure. */ -int +void
Well this doesn't jive with the comment above about what it returns...
virNetworkObjAssignDef(virNetworkObjPtr network, virNetworkDefPtr def, bool live) { - if (virNetworkObjIsActive(network)) { - if (live) { + if (live) { + /* before setting new live def, save (into newDef) any + * existing persistent (!live) def to be restored when the + * network is destroyed, unless there is one already saved. + */ + if (network->persistent && !network->newDef) + network->newDef = network->def; + else virNetworkDefFree(network->def); - network->def = def; - } else if (network->persistent) { - /* save current configuration to be restored on network shutdown */ - virNetworkDefFree(network->newDef); + network->def = def; + } else { /* !live */ + virNetworkDefFree(network->newDef); + if (virNetworkObjIsActive(network)) { + /* save new configuration to be restored on network + * shutdown, leaving current live def alone + */ network->newDef = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save persistent config of transient " - "network '%s'"), network->def->name); - return -1; + } else { /* !live and !active */ + if (network->def && !network->persistent) { + /* network isn't (yet) marked active or persistent, + * but already has a "live" def set. This means we are + * currently setting the persistent def as a part of + * the process of starting the network, so we need to + * preserve the "not yet live" def in network->def. + */ + network->newDef = def; + } else { + /* either there is no live def set, or this network + * was already set as persistent, so the proper thing + * is to overwrite network->def. + */ + network->newDef = NULL;
If I'm reading correctly - the newDef is virNetworkDefFree()'d above as we enter the "} else { /* !live */" clause, which is probably where this statement should be moved.
+ virNetworkDefFree(network->def); + network->def = def;
NOTE: I think this is what is causing issues in networkUndefine() - that'll call this function with '!live' - thus you'll do the free the 'def' and set it to NULL... Back in the caller there's all sorts of touching of the network->def-> structure
+ } } - } else if (!live) { - virNetworkDefFree(network->newDef); - virNetworkDefFree(network->def); - network->newDef = NULL; - network->def = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save live config of inactive " - "network '%s'"), network->def->name); - return -1; + network->persistent = !!def; } - return 0; }
/* @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkObjPtr network;
if ((network = virNetworkFindByName(nets, def->name))) { - if (virNetworkObjAssignDef(network, def, live) < 0) { - virNetworkObjUnlock(network); - return NULL; - } + virNetworkObjAssignDef(network, def, live); return network; }
@@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets, ignore_value(virBitmapSetBit(network->class_id, 2));
network->def = def; - + network->persistent = !live; return network; + error: virNetworkObjUnlock(network); virNetworkObjFree(network); @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, goto error;
net->autostart = autostart; - net->persistent = 1;
VIR_FREE(configFile); VIR_FREE(autostartLink); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1a864de..90da16f 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefPtr def, bool live); -int virNetworkObjAssignDef(virNetworkObjPtr network, - virNetworkDefPtr def, - bool live); +void virNetworkObjAssignDef(virNetworkObjPtr network, + virNetworkDefPtr def, + bool live); int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live); void virNetworkObjUnsetDefTransient(virNetworkObjPtr network); virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index eb276cd..62d1c25 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, true) < 0) goto cleanup;
- /* NB: "live" is false because this transient network hasn't yet - * been started + /* NB: even though this transient network hasn't yet been started, + * we assign the def with live = true in anticipation that it will + * be started momentarily. */ - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + if (!(network = virNetworkAssignDef(&driver->networks, def, true))) goto cleanup; def = NULL;
@@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, false) < 0) goto cleanup;
- if ((network = virNetworkFindByName(&driver->networks, def->name))) { - network->persistent = 1; - if (virNetworkObjAssignDef(network, def, false) < 0) - goto cleanup; - } else { - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) - goto cleanup; - } - - /* define makes the network persistent - always */ - network->persistent = 1; + if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + goto cleanup;
- /* def was asigned */ + /* def was assigned to network object */ freeDef = false;
if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) network = NULL; goto cleanup; } - network->persistent = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + /* if network was active already, just undo new persistent + * definition by making it transient. + * XXX - this isn't necessarily the correct thing to do. + */
Leaving the XXX comment in here leaves me wondering what is the correct thing to do...
+ virNetworkObjAssignDef(network, NULL, false); goto cleanup; }
@@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net) goto cleanup;
/* make the network transient */ - network->persistent = 0; network->autostart = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + virNetworkObjAssignDef(network, NULL, false);
This seems to be the cause of the crash I noted in private chat. If I restore just these lines and not make the ObjAssignDef() call, then things work again. That includes issues I was seeing in virt-test for patches 3 & 4 as well. Just figured I'd close the loop on this review at least... John
event = virNetworkEventLifecycleNew(network->def->name, network->def->uuid, diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index cbb44b9..0ebeb7b 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -2,7 +2,7 @@ * parallels_network.c: core privconn functions for managing * Parallels Cloud Server hosts * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2014 Red Hat, Inc. * Copyright (C) 2012 Parallels, Inc. * * This library is free software; you can redistribute it and/or @@ -231,7 +231,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; } net->active = 1; - net->persistent = 1; net->autostart = 1; virNetworkObjUnlock(net); return net; @@ -267,7 +266,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) goto cleanup; } net->active = 1; - net->persistent = 1; net->autostart = 1; virNetworkObjUnlock(net);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 64f3daa..37756e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -784,7 +784,6 @@ testOpenDefault(virConnectPtr conn) goto error; } netobj->active = 1; - netobj->persistent = 1; virNetworkObjUnlock(netobj);
if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML))) @@ -1155,7 +1154,6 @@ testParseNetworks(testConnPtr privconn, goto error; }
- obj->persistent = 1; obj->active = 1; virNetworkObjUnlock(obj); } @@ -3711,7 +3709,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup;
- if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) + if (!(net = virNetworkAssignDef(&privconn->networks, def, true))) goto cleanup; def = NULL; net->active = 1; @@ -3748,7 +3746,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) goto cleanup; def = NULL; - net->persistent = 1;
event = virNetworkEventLifecycleNew(net->def->name, net->def->uuid, VIR_NETWORK_EVENT_DEFINED,
On 04/24/2014 07:09 PM, John Ferlan wrote:
On 04/23/2014 09:49 AM, Laine Stump wrote:
Experimentation showed that if virNetworkCreateXML() was called for a network that was already defined, and then the network was subsequently shutdown, the network would continue to be persistent after the shutdown (expected/desired), but the original config would be lost in favor of the transient config sent in with virNetworkCreateXML() (which would then be the new persistent config) (obviously unexpected/not desired).
To fix this, virNetworkObjAssignDef() has been changed to
1) properly save/free network->def and network->newDef for all the various combinations of live/active/persistent, including some combinations that were previously considered to be an error but didn't need to be (e.g. setting a "live" config for a network that isn't yet active but soon will be - that was previously considered an error, even though in practice it can be very useful).
2) automatically set the persistent flag whenever a new non-live config is assigned to the network (and clear it when the non-live config is set to NULL). the libvirt network driver no longer directly manipulates network->persistent, but instead relies entirely on virNetworkObjAssignDef() to do the right thing automatically.
After this patch, the following sequence will behave as expected:
virNetworkDefineXML(X) virNetworkCreateXML(X') (same name but some config different) virNetworkDestroy(X)
At the end of these calls, the network config will remain as it was after the initial virNetworkDefine(), whereas previously it would take on the changes given during virNetworkCreateXML().
Another effect of this tighter coupling between a) setting a !live def and b) setting/clearing the "persistent" flag, is that future patches which change the details of network lifecycle management (e.g. upcoming patches to fix detection of "active" networks when libvirtd is restarted) will find it much more difficult to break persistence functionality. --- src/conf/network_conf.c | 78 +++++++++++++++++++++++---------------- src/conf/network_conf.h | 6 +-- src/network/bridge_driver.c | 34 +++++++---------- src/parallels/parallels_network.c | 4 +- src/test/test_driver.c | 5 +-- 5 files changed, 64 insertions(+), 63 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..66945ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets) /* * virNetworkObjAssignDef: * @network: the network object to update - * @def: the new NetworkDef (will be consumed by this function iff successful) + * @def: the new NetworkDef (will be consumed by this function) * @live: is this new def the "live" version, or the "persistent" version * - * Replace the appropriate copy of the given network's NetworkDef + * Replace the appropriate copy of the given network's def or newDef * with def. Use "live" and current state of the network to determine - * which to replace. + * which to replace and what to do with the old defs. When a non-live + * def is set, indicate that the network is now persistent. + * + * NB: a persistent network can be made transient by calling with: + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the + * persistent def to NULL) * * Returns 0 on success, -1 on failure. */ -int +void Well this doesn't jive with the comment above about what it returns...
Yeah, when all the failure paths disappeared I changed the function to void, but forgot to remove that comment. I'll fix that now.
virNetworkObjAssignDef(virNetworkObjPtr network, virNetworkDefPtr def, bool live) { - if (virNetworkObjIsActive(network)) { - if (live) { + if (live) { + /* before setting new live def, save (into newDef) any + * existing persistent (!live) def to be restored when the + * network is destroyed, unless there is one already saved. + */ + if (network->persistent && !network->newDef) + network->newDef = network->def; + else virNetworkDefFree(network->def); - network->def = def; - } else if (network->persistent) { - /* save current configuration to be restored on network shutdown */ - virNetworkDefFree(network->newDef); + network->def = def; + } else { /* !live */ + virNetworkDefFree(network->newDef); + if (virNetworkObjIsActive(network)) { + /* save new configuration to be restored on network + * shutdown, leaving current live def alone + */ network->newDef = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save persistent config of transient " - "network '%s'"), network->def->name); - return -1; + } else { /* !live and !active */ + if (network->def && !network->persistent) { + /* network isn't (yet) marked active or persistent, + * but already has a "live" def set. This means we are + * currently setting the persistent def as a part of + * the process of starting the network, so we need to + * preserve the "not yet live" def in network->def. + */ + network->newDef = def; + } else { + /* either there is no live def set, or this network + * was already set as persistent, so the proper thing + * is to overwrite network->def. + */ + network->newDef = NULL; If I'm reading correctly - the newDef is virNetworkDefFree()'d above as we enter the "} else { /* !live */" clause, which is probably where this statement should be moved.
No, I just did some reduction of common code by moving it above the if statement. You'll notice that network->newDef is set to *something* in every path through those ifs and elses, so at the end of the function, it is either NULL or pointing to a valid object.
+ virNetworkDefFree(network->def); + network->def = def;
NOTE: I think this is what is causing issues in networkUndefine() - that'll call this function with '!live' - thus you'll do the free the 'def' and set it to NULL... Back in the caller there's all sorts of touching of the network->def-> structure
Yes, indirectly. But the *real* problem is that we shouldn't be calling this function until after we're finished using anything in network->def. See below for the small diff I'm squashing in to fix this.
+ } } - } else if (!live) { - virNetworkDefFree(network->newDef); - virNetworkDefFree(network->def); - network->newDef = NULL; - network->def = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save live config of inactive " - "network '%s'"), network->def->name); - return -1; + network->persistent = !!def; } - return 0; }
/* @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkObjPtr network;
if ((network = virNetworkFindByName(nets, def->name))) { - if (virNetworkObjAssignDef(network, def, live) < 0) { - virNetworkObjUnlock(network); - return NULL; - } + virNetworkObjAssignDef(network, def, live); return network; }
@@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets, ignore_value(virBitmapSetBit(network->class_id, 2));
network->def = def; - + network->persistent = !live; return network; + error: virNetworkObjUnlock(network); virNetworkObjFree(network); @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, goto error;
net->autostart = autostart; - net->persistent = 1;
VIR_FREE(configFile); VIR_FREE(autostartLink); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1a864de..90da16f 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefPtr def, bool live); -int virNetworkObjAssignDef(virNetworkObjPtr network, - virNetworkDefPtr def, - bool live); +void virNetworkObjAssignDef(virNetworkObjPtr network, + virNetworkDefPtr def, + bool live); int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live); void virNetworkObjUnsetDefTransient(virNetworkObjPtr network); virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index eb276cd..62d1c25 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, true) < 0) goto cleanup;
- /* NB: "live" is false because this transient network hasn't yet - * been started + /* NB: even though this transient network hasn't yet been started, + * we assign the def with live = true in anticipation that it will + * be started momentarily. */ - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + if (!(network = virNetworkAssignDef(&driver->networks, def, true))) goto cleanup; def = NULL;
@@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, false) < 0) goto cleanup;
- if ((network = virNetworkFindByName(&driver->networks, def->name))) { - network->persistent = 1; - if (virNetworkObjAssignDef(network, def, false) < 0) - goto cleanup; - } else { - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) - goto cleanup; - } - - /* define makes the network persistent - always */ - network->persistent = 1; + if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + goto cleanup;
- /* def was asigned */ + /* def was assigned to network object */ freeDef = false;
if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) network = NULL; goto cleanup; } - network->persistent = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + /* if network was active already, just undo new persistent + * definition by making it transient. + * XXX - this isn't necessarily the correct thing to do. + */ Leaving the XXX comment in here leaves me wondering what is the correct thing to do...
The correct thing to do is... well, to exactly rollback to what you had before the failure. However, the current code (both before *and* after this patch) does the (same) wrong thing in this case - if you fail during an attempt to redefine an already persistent+active network, you'll end up with a transient+active network (so it will disappear as soon as the network is stopped). It would be better if such a failure caused us to revert back to the previous persistent def. That would require saving off the existing def before trying to assign the new one or writing to disk. But that is a separate problem and should be in a separate patch anyway. The only reason I touched that area of the code in this patch was that I was replacing direct assignment of network->persistent with calling virNetworkObjAssignDef() (while preserving the existing functionality). I did think it was worth noting the problem with the existing functionality while touching the code, however. So, while I think this needs fixing, it's pre-existing and I'd rather not have these other patches delayed because of it.
+ virNetworkObjAssignDef(network, NULL, false); goto cleanup; }
@@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net) goto cleanup;
/* make the network transient */ - network->persistent = 0; network->autostart = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + virNetworkObjAssignDef(network, NULL, false); This seems to be the cause of the crash I noted in private chat. If I restore just these lines and not make the ObjAssignDef() call, then things work again. That includes issues I was seeing in virt-test for patches 3 & 4 as well.
Yep. That call needs to be moved down to below the call to networkRemoveInactive. I've done that and all the tests complete without failure (as you've also separately reported on IRC). Here is what I squashed into this patch: diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 309d967..2a54aa3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2899,14 +2899,12 @@ networkUndefine(virNetworkPtr net) if (virNetworkObjIsActive(network)) active = true; + /* remove autostart link */ if (virNetworkDeleteConfig(driver->networkConfigDir, driver->networkAutostartDir, network) < 0) goto cleanup; - - /* make the network transient */ network->autostart = 0; - virNetworkObjAssignDef(network, NULL, false); event = virNetworkEventLifecycleNew(network->def->name, network->def->uuid, @@ -2920,6 +2918,12 @@ networkUndefine(virNetworkPtr net) goto cleanup; } network = NULL; + } else { + + /* if the network still exists, it was active, and we need to make + * it transient (by deleting the persistent def + */ + virNetworkObjAssignDef(network, NULL, false); } ret = 0; --
Just figured I'd close the loop on this review at least...
Do my responses clear up your concerns?
On 04/25/2014 01:55 PM, Laine Stump wrote:
On 04/24/2014 07:09 PM, John Ferlan wrote:
On 04/23/2014 09:49 AM, Laine Stump wrote:
Experimentation showed that if virNetworkCreateXML() was called for a network that was already defined, and then the network was subsequently shutdown, the network would continue to be persistent after the shutdown (expected/desired), but the original config would be lost in favor of the transient config sent in with virNetworkCreateXML() (which would then be the new persistent config) (obviously unexpected/not desired).
To fix this, virNetworkObjAssignDef() has been changed to
1) properly save/free network->def and network->newDef for all the various combinations of live/active/persistent, including some combinations that were previously considered to be an error but didn't need to be (e.g. setting a "live" config for a network that isn't yet active but soon will be - that was previously considered an error, even though in practice it can be very useful).
2) automatically set the persistent flag whenever a new non-live config is assigned to the network (and clear it when the non-live config is set to NULL). the libvirt network driver no longer directly manipulates network->persistent, but instead relies entirely on virNetworkObjAssignDef() to do the right thing automatically.
After this patch, the following sequence will behave as expected:
virNetworkDefineXML(X) virNetworkCreateXML(X') (same name but some config different) virNetworkDestroy(X)
At the end of these calls, the network config will remain as it was after the initial virNetworkDefine(), whereas previously it would take on the changes given during virNetworkCreateXML().
Another effect of this tighter coupling between a) setting a !live def and b) setting/clearing the "persistent" flag, is that future patches which change the details of network lifecycle management (e.g. upcoming patches to fix detection of "active" networks when libvirtd is restarted) will find it much more difficult to break persistence functionality. --- src/conf/network_conf.c | 78 +++++++++++++++++++++++---------------- src/conf/network_conf.h | 6 +-- src/network/bridge_driver.c | 34 +++++++---------- src/parallels/parallels_network.c | 4 +- src/test/test_driver.c | 5 +-- 5 files changed, 64 insertions(+), 63 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..66945ce 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -302,46 +302,63 @@ void virNetworkObjListFree(virNetworkObjListPtr nets) /* * virNetworkObjAssignDef: * @network: the network object to update - * @def: the new NetworkDef (will be consumed by this function iff successful) + * @def: the new NetworkDef (will be consumed by this function) * @live: is this new def the "live" version, or the "persistent" version * - * Replace the appropriate copy of the given network's NetworkDef + * Replace the appropriate copy of the given network's def or newDef * with def. Use "live" and current state of the network to determine - * which to replace. + * which to replace and what to do with the old defs. When a non-live + * def is set, indicate that the network is now persistent. + * + * NB: a persistent network can be made transient by calling with: + * virNetworkObjAssignDef(network, NULL, false) (i.e. set the + * persistent def to NULL) * * Returns 0 on success, -1 on failure. */ -int +void Well this doesn't jive with the comment above about what it returns...
Yeah, when all the failure paths disappeared I changed the function to void, but forgot to remove that comment. I'll fix that now.
virNetworkObjAssignDef(virNetworkObjPtr network, virNetworkDefPtr def, bool live) { - if (virNetworkObjIsActive(network)) { - if (live) { + if (live) { + /* before setting new live def, save (into newDef) any + * existing persistent (!live) def to be restored when the + * network is destroyed, unless there is one already saved. + */ + if (network->persistent && !network->newDef) + network->newDef = network->def; + else virNetworkDefFree(network->def); - network->def = def; - } else if (network->persistent) { - /* save current configuration to be restored on network shutdown */ - virNetworkDefFree(network->newDef); + network->def = def; + } else { /* !live */ + virNetworkDefFree(network->newDef); + if (virNetworkObjIsActive(network)) { + /* save new configuration to be restored on network + * shutdown, leaving current live def alone + */ network->newDef = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save persistent config of transient " - "network '%s'"), network->def->name); - return -1; + } else { /* !live and !active */ + if (network->def && !network->persistent) { + /* network isn't (yet) marked active or persistent, + * but already has a "live" def set. This means we are + * currently setting the persistent def as a part of + * the process of starting the network, so we need to + * preserve the "not yet live" def in network->def. + */ + network->newDef = def; + } else { + /* either there is no live def set, or this network + * was already set as persistent, so the proper thing + * is to overwrite network->def. + */ + network->newDef = NULL; If I'm reading correctly - the newDef is virNetworkDefFree()'d above as we enter the "} else { /* !live */" clause, which is probably where this statement should be moved.
No, I just did some reduction of common code by moving it above the if statement. You'll notice that network->newDef is set to *something* in every path through those ifs and elses, so at the end of the function, it is either NULL or pointing to a valid object.
+ virNetworkDefFree(network->def); + network->def = def;
NOTE: I think this is what is causing issues in networkUndefine() - that'll call this function with '!live' - thus you'll do the free the 'def' and set it to NULL... Back in the caller there's all sorts of touching of the network->def-> structure
Yes, indirectly. But the *real* problem is that we shouldn't be calling this function until after we're finished using anything in network->def. See below for the small diff I'm squashing in to fix this.
+ } } - } else if (!live) { - virNetworkDefFree(network->newDef); - virNetworkDefFree(network->def); - network->newDef = NULL; - network->def = def; - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - _("cannot save live config of inactive " - "network '%s'"), network->def->name); - return -1; + network->persistent = !!def; } - return 0; }
/* @@ -365,10 +382,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkObjPtr network;
if ((network = virNetworkFindByName(nets, def->name))) { - if (virNetworkObjAssignDef(network, def, live) < 0) { - virNetworkObjUnlock(network); - return NULL; - } + virNetworkObjAssignDef(network, def, live); return network; }
@@ -392,8 +406,9 @@ virNetworkAssignDef(virNetworkObjListPtr nets, ignore_value(virBitmapSetBit(network->class_id, 2));
network->def = def; - + network->persistent = !live; return network; + error: virNetworkObjUnlock(network); virNetworkObjFree(network); @@ -3118,7 +3133,6 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, goto error;
net->autostart = autostart; - net->persistent = 1;
VIR_FREE(configFile); VIR_FREE(autostartLink); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1a864de..90da16f 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -335,9 +335,9 @@ typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefPtr def, bool live); -int virNetworkObjAssignDef(virNetworkObjPtr network, - virNetworkDefPtr def, - bool live); +void virNetworkObjAssignDef(virNetworkObjPtr network, + virNetworkDefPtr def, + bool live); int virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live); void virNetworkObjUnsetDefTransient(virNetworkObjPtr network); virNetworkDefPtr virNetworkObjGetPersistentDef(virNetworkObjPtr network); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index eb276cd..62d1c25 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2667,10 +2667,11 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, true) < 0) goto cleanup;
- /* NB: "live" is false because this transient network hasn't yet - * been started + /* NB: even though this transient network hasn't yet been started, + * we assign the def with live = true in anticipation that it will + * be started momentarily. */ - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + if (!(network = virNetworkAssignDef(&driver->networks, def, true))) goto cleanup; def = NULL;
@@ -2719,19 +2720,10 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (networkValidate(driver, def, false) < 0) goto cleanup;
- if ((network = virNetworkFindByName(&driver->networks, def->name))) { - network->persistent = 1; - if (virNetworkObjAssignDef(network, def, false) < 0) - goto cleanup; - } else { - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) - goto cleanup; - } - - /* define makes the network persistent - always */ - network->persistent = 1; + if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + goto cleanup;
- /* def was asigned */ + /* def was assigned to network object */ freeDef = false;
if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { @@ -2740,9 +2732,11 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) network = NULL; goto cleanup; } - network->persistent = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + /* if network was active already, just undo new persistent + * definition by making it transient. + * XXX - this isn't necessarily the correct thing to do. + */ Leaving the XXX comment in here leaves me wondering what is the correct thing to do...
The correct thing to do is... well, to exactly rollback to what you had before the failure.
However, the current code (both before *and* after this patch) does the (same) wrong thing in this case - if you fail during an attempt to redefine an already persistent+active network, you'll end up with a transient+active network (so it will disappear as soon as the network is stopped).
It would be better if such a failure caused us to revert back to the previous persistent def. That would require saving off the existing def before trying to assign the new one or writing to disk. But that is a separate problem and should be in a separate patch anyway. The only reason I touched that area of the code in this patch was that I was replacing direct assignment of network->persistent with calling virNetworkObjAssignDef() (while preserving the existing functionality). I did think it was worth noting the problem with the existing functionality while touching the code, however.
So, while I think this needs fixing, it's pre-existing and I'd rather not have these other patches delayed because of it.
+ virNetworkObjAssignDef(network, NULL, false); goto cleanup; }
@@ -2794,10 +2788,8 @@ networkUndefine(virNetworkPtr net) goto cleanup;
/* make the network transient */ - network->persistent = 0; network->autostart = 0; - virNetworkDefFree(network->newDef); - network->newDef = NULL; + virNetworkObjAssignDef(network, NULL, false); This seems to be the cause of the crash I noted in private chat. If I restore just these lines and not make the ObjAssignDef() call, then things work again. That includes issues I was seeing in virt-test for patches 3 & 4 as well.
Yep. That call needs to be moved down to below the call to networkRemoveInactive. I've done that and all the tests complete without failure (as you've also separately reported on IRC). Here is what I squashed into this patch:
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 309d967..2a54aa3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2899,14 +2899,12 @@ networkUndefine(virNetworkPtr net) if (virNetworkObjIsActive(network)) active = true;
+ /* remove autostart link */ if (virNetworkDeleteConfig(driver->networkConfigDir, driver->networkAutostartDir, network) < 0) goto cleanup; - - /* make the network transient */ network->autostart = 0; - virNetworkObjAssignDef(network, NULL, false);
event = virNetworkEventLifecycleNew(network->def->name, network->def->uuid, @@ -2920,6 +2918,12 @@ networkUndefine(virNetworkPtr net) goto cleanup; } network = NULL; + } else { + + /* if the network still exists, it was active, and we need to make + * it transient (by deleting the persistent def
s/def/def)/
+ */ + virNetworkObjAssignDef(network, NULL, false); }
ret = 0; --
Just figured I'd close the loop on this review at least...
Do my responses clear up your concerns?
Yep - all set, thanks for the extra details... You have an ACK with that extra ) above John
participants (3)
-
Eric Blake -
John Ferlan -
Laine Stump