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. --- 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..fdddc9a 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 cleanup; + } + /* 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

For some reason these have been stored in /var/lib, although other drivers store their state files in /var/run. It's much nicer to store them in /var/run because they are automatically cleared out when the system reboots. This can be a useful way of learning whether or not a particular network is active. Note that this change by itself will cause problems in the case of an upgrade from an older libvirt that uses /var/lib, and also in the case of a downgrade from a newer libvirt using /var/run to an older version using /var/lib. This compatibility problem will be addressed in the next patch. --- src/network/bridge_driver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fdddc9a..a027b47 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -446,10 +446,11 @@ networkStateInitialize(bool privileged, * ~/.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. + * NB: The network driver previously stored its status xml in + * /var/lib/libvirt/network instead of + * /var/run/libvirt/network. An upgrade downgrade between libvirt + * versions using different state directories may or may not cause + * problems. */ if (privileged) { if (VIR_STRDUP(driverState->networkConfigDir, @@ -457,7 +458,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, -- 1.9.0

On 04/15/2014 03:26 PM, John Ferlan wrote: Correct. Because that else clause for non-privileged was added long after the original code for privileged was written, and the directories are already in the correct place from the beginning so they don't need to be fixed. I'm actually not sure exactly what is done with the directories in ~/.config (when/if the $run directory is cleared, etc), but there was some sort of reconfig/standardization of the locations with gnome3 that the qemu driver was made to follow, and the network driver copies those locations faithfully (replacing "qemu" with "network" or "qemu/network" as appropriate). non-privileged libvirtd is a bit odd. For starters it isn't run until someone tries to connect to their user's qemu:///session, and then it automatically exits after some timeout. Beyond that, the network driver doesn't actually work (!!) for non-privileged libvirtd - adding the ~/.config directories was only done to prevent it from crashing, with the thinking that if we *did* make it work sometime in the future, those would be the right places to store the files (since it works for qemu...) Well, yes and no. Both patches independently pass make check and make syntax-check, and I hadn't considered that anyone would do a bisect where the test at each step was to install an older libvirt, then upgrade to the newly built bisect binary. However, my main reason for separating the two was just to make review easier (as well as in case I decided to make any more complicated alternative to 3/5), and there's no point in dooming such a bisect test to failure for no good reason, so I would be fine with merging 2/5 and 3/5.

On 04/10/2014 09:19 AM, Laine Stump wrote: I would have to believe/think the downgrade issue has happened before with some other change to config or status file locations. Of course, if some state/status is meant to "live" beyond the current execution, then perhaps /run/ isn't the right place to store that. Wouldn't something like that end up in /etc/libvirt... somewhere? That is usage of '--live' vs. '--config' when configuring limits? You'll need a corresponding VIR_FREE() in networkStateCleanup() ahh... so this isn't done for the non privileged case... So if privileged and the network state exists in the old state dir... Also could use STRNEQ_NULLABLE() right? So any/all failures to read the old entry/style causes failure... Could this be problematic if something ends up existing in the "new" format that isn't in the "old" format and thus causes exit by the changed parsing code? This is perhaps the opposite problem to flawless downgrades. It also points out the question of how long do we keep this old reading code around before it's removed? Alternatively to removing - rename to some suffix which at least gives a chance for someone going through a downgrade process to recover the old file. Of course documenting this would be nice too. This hunk should be it's own local/static function... ACK pending your thoughts regarding my comments... John

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 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 libirt driver that stores status xml for its objects). The difference is that /var/run is cleared out when the host reboots, so you cvqan 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 networkFindInactiveConfigs(), 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 it is restarted, and network with a status file will be marked as inactive (unless it uses a bridge device and that device for some reason doesn't exist). --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 49 +++++++++++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 13 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 98d10bd..7beab79 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -328,7 +328,7 @@ networkBridgeDummyNicName(const char *brname) } static void -networkFindActiveConfigs(virNetworkDriverStatePtr driver) +networkFindInactiveConfigs(virNetworkDriverStatePtr driver) { size_t i; @@ -336,29 +336,51 @@ networkFindActiveConfigs(virNetworkDriverStatePtr driver) virNetworkObjPtr obj = driver->networks.objs[i]; 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; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + obj->def->name, &obj->dnsmasqPid, dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - - if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) - goto cleanup; - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + break; + ignore_value(virPidFileReadIfAlive(driverState->pidDir, + radvdpidbase, &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase); } + break; + + 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: + case VIR_NETWORK_FORWARD_HOSTDEV: + break; } - cleanup: virNetworkObjUnlock(obj); } @@ -542,12 +564,13 @@ networkStateInitialize(bool privileged, driverState->stateDir) < 0) goto error; + networkFindInactiveConfigs(driverState); + if (virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir) < 0) goto error; - networkFindActiveConfigs(driverState); networkReloadFirewallRules(driverState); networkRefreshDaemons(driverState); -- 1.9.0

On 04/15/2014 03:31 PM, John Ferlan wrote: Yeah, the original code assumed active == 0, and set active = 1 if the bridge existed. I forgot to change the comments. Correct. Previously the basic assumption was that all networks were inactive when libvirtd restarted and we would only mark them active if they had a bridge and it existed. Since some network types don't use a bridge, this is now inadequate, and there's no other external info we can use to make this determination, so instead we assume that any network which has a state file (that's why the function now runs *before* loading persistent configs - so the list only contains those with state files) is active, and then we change the polarity of the function checking external info to set a network inactive if the check fails. This is a case of evolution of the code - setting of the active flag and refreshing the pids from the pidfiles have been together in the same function since "a long time ago", as they both happened during driver initialization, and both required cycling through the list of all network objects. Perhaps it would be less offensive if the function name was changed from networkFindInactiveConfigs() to networkUpdateAllState() (or something like that). The idea is that we've just read the state of all networks on disk, and this function will update the results with any external information from the host, e.g. if a bridge has disappeared then the network must be marked down, and the pids (which are not stored in the state file as that would be redundant information) are reread from the pidfiles. Also, I thought I had seen you wondering somewhere about the fact that the check for pidfiles was previously done for all active networks, but now it is only done for networks with forward mode of none, route, or nat. While the previous code ended up behaving properly, this was accidental - none of the other types of networks use dnsmasq or radvd, so they would never have a pidfile. Yes. "config" is the persistent config, which is completely self-contained in the xml file in /etc/libvirt/qemu/network, but the pids are part of the network's state. You mean the pids? Nope, those are unrelated to the config.

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 4 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 4 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 4 patches as soon as I have pushed them.] This fixes: https://bugzilla.redhat.com/show_bug.cgi?id=880483 --- 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 942e139..f84b8c3 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 7beab79..132af4f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3118,7 +3118,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; } @@ -3462,6 +3463,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 379c094..baf1ad1 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/15/2014 03:31 PM, John Ferlan wrote: Yes, the behavior change is intentional, and checking for "actualType" will always be okay at any point past this since, as you imply, if networkAllocateActualDevice() had failed, we wouldn't even get to any of the later checks of actualType.