Re: [libvirt] [PATCHv2] network: fix network driver startup for qemu:///session

This should resolve https://bugzilla.redhat.com/show_bug.cgi?id=958907 Recent new addition of code to read/write active network state to the NETWORK_STATE_DIR in the network driver broke startup for qemu:///session. The network driver had several state file paths hardcoded to /var, which could never possibly work in session mode. This patch modifies *all* state files to use a variable string that is set differently according to whether or not we're running privileged. (It turns out that logDir was never used, so it's been completely eliminated.) There are very definitely other problems preventing dnsmasq and radvd from running in non-privileged mode, but it's more consistent to have the directories used by them be determined in the same fashion. NB: I've noted before that the network driver is storing its state (including dnsmasq and radvd state) in /var/lib, while qemu stores its state in /var/run. It would probably have been better if the two matched, but it's been this way for a long time, and changing it would break running installations during an upgrade, so it's best to just leave it as it is. --- Changes since V1: * change user directory names as outlined by danpb. * eliminate the "base" string which caused so much bad code, and otherwise simplify the logic * get rid of logDir, since it's never used. * eliminage the *_DIR #defines, since they're now each only used once, and they just serve to obscure what's being done. src/network/bridge_driver.c | 182 +++++++++++++++++++++++++------------------- 1 file changed, 102 insertions(+), 80 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e828997..543b098 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1,4 +1,3 @@ - /* * bridge_driver.c: core driver methods for managing network * @@ -67,12 +66,6 @@ #include "virfile.h" #include "virstring.h" -#define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" -#define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" - -#define DNSMASQ_STATE_DIR LOCALSTATEDIR "/lib/libvirt/dnsmasq" -#define RADVD_STATE_DIR LOCALSTATEDIR "/lib/libvirt/radvd" - #define VIR_FROM_THIS VIR_FROM_NETWORK /* Main driver state */ @@ -84,7 +77,10 @@ struct network_driver { iptablesContext *iptables; char *networkConfigDir; char *networkAutostartDir; - char *logDir; + char *stateDir; + char *pidDir; + char *dnsmasqStateDir; + char *radvdStateDir; dnsmasqCapsPtr dnsmasqCaps; }; @@ -133,8 +129,8 @@ networkDnsmasqLeaseFileNameDefault(const char *netname) { char *leasefile; - ignore_value(virAsprintf(&leasefile, DNSMASQ_STATE_DIR "/%s.leases", - netname)); + ignore_value(virAsprintf(&leasefile, "%s/%s.leases", + driverState->dnsmasqStateDir, netname)); return leasefile; } @@ -146,8 +142,8 @@ networkDnsmasqConfigFileName(const char *netname) { char *conffile; - ignore_value(virAsprintf(&conffile, DNSMASQ_STATE_DIR "/%s.conf", - netname)); + ignore_value(virAsprintf(&conffile, "%s/%s.conf", + driverState->dnsmasqStateDir, netname)); return conffile; } @@ -166,8 +162,8 @@ networkRadvdConfigFileName(const char *netname) { char *configfile; - ignore_value(virAsprintf(&configfile, RADVD_STATE_DIR "/%s-radvd.conf", - netname)); + ignore_value(virAsprintf(&configfile, "%s/%s-radvd.conf", + driverState->radvdStateDir, netname)); return configfile; } @@ -187,8 +183,10 @@ networkRemoveInactive(struct network_driver *driver, int ret = -1; /* remove the (possibly) existing dnsmasq and radvd files */ - if (!(dctx = dnsmasqContextNew(def->name, DNSMASQ_STATE_DIR))) + if (!(dctx = dnsmasqContextNew(def->name, + driverState->dnsmasqStateDir))) { goto cleanup; + } if (!(leasefile = networkDnsmasqLeaseFileName(def->name))) goto cleanup; @@ -202,7 +200,8 @@ networkRemoveInactive(struct network_driver *driver, if (!(configfile = networkDnsmasqConfigFileName(def->name))) goto no_memory; - if (!(statusfile = virNetworkConfigFile(NETWORK_STATE_DIR, def->name))) + if (!(statusfile + = virNetworkConfigFile(driverState->stateDir, def->name))) goto no_memory; /* dnsmasq */ @@ -212,7 +211,7 @@ networkRemoveInactive(struct network_driver *driver, /* radvd */ unlink(radvdconfigfile); - virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); + virPidFileDelete(driverState->pidDir, radvdpidbase); /* remove status file */ unlink(statusfile); @@ -279,7 +278,7 @@ networkFindActiveConfigs(struct network_driver *driver) if (obj->def->ips && (obj->def->nips > 0)) { char *radvdpidbase; - ignore_value(virPidFileReadIfAlive(NETWORK_PID_DIR, obj->def->name, + ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name, &obj->dnsmasqPid, dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); @@ -287,7 +286,7 @@ networkFindActiveConfigs(struct network_driver *driver) virReportOOMError(); goto cleanup; } - ignore_value(virPidFileReadIfAlive(NETWORK_PID_DIR, radvdpidbase, + ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase, &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase); } @@ -359,7 +358,9 @@ networkStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - char *base = NULL; + int ret = -1; + char *configdir = NULL; + char *rundir = NULL; #ifdef HAVE_FIREWALLD DBusConnection *sysbus = NULL; #endif @@ -373,43 +374,53 @@ networkStateInitialize(bool privileged, } networkDriverLock(driverState); + /* Configuration paths one of + * ~/.libvirt/... (old style session/unprivileged) + * ~/.config/libvirt/... (new XDG 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 should change it now. + */ if (privileged) { - if (virAsprintf(&driverState->logDir, - "%s/log/libvirt/qemu", LOCALSTATEDIR) == -1) - goto out_of_memory; - - if ((base = strdup(SYSCONFDIR "/libvirt")) == NULL) + if (!(driverState->networkConfigDir + = strdup(SYSCONFDIR "/libvirt/qemu/networks")) || + !(driverState->networkAutostartDir + = strdup(SYSCONFDIR "/libvirt/qemu/networks/autostart")) || + !(driverState->stateDir + = strdup(LOCALSTATEDIR "/lib/libvirt/network")) || + !(driverState->pidDir + = strdup(LOCALSTATEDIR "/run/libvirt/network")) || + !(driverState->dnsmasqStateDir + = strdup(LOCALSTATEDIR "/lib/libvirt/dnsmasq")) || + !(driverState->radvdStateDir + = strdup(LOCALSTATEDIR "/lib/libvirt/radvd"))) { goto out_of_memory; + } } else { - char *userdir = virGetUserCacheDirectory(); - - if (!userdir) + configdir = virGetUserConfigDirectory(); + rundir = virGetUserRuntimeDirectory(); + if (!(configdir && rundir)) goto error; - if (virAsprintf(&driverState->logDir, - "%s/qemu/log", userdir) == -1) { - VIR_FREE(userdir); + if ((virAsprintf(&driverState->networkConfigDir, + "%s/qemu/networks", configdir) < 0) || + (virAsprintf(&driverState->networkAutostartDir, + "%s/qemu/networks/autostart", configdir) < 0) || + (virAsprintf(&driverState->stateDir, + "%s/network/lib", rundir) < 0) || + (virAsprintf(&driverState->pidDir, + "%s/network/run", rundir) < 0) || + (virAsprintf(&driverState->dnsmasqStateDir, + "%s/dnsmasq/lib", rundir) < 0) || + (virAsprintf(&driverState->radvdStateDir, + "%s/radvd/lib", rundir) < 0)) { goto out_of_memory; } - VIR_FREE(userdir); - - base = virGetUserConfigDirectory(); - if (!base) - goto error; } - /* Configuration paths are either ~/.libvirt/qemu/... (session) or - * /etc/libvirt/qemu/... (system). - */ - if (virAsprintf(&driverState->networkConfigDir, "%s/qemu/networks", base) == -1) - goto out_of_memory; - - if (virAsprintf(&driverState->networkAutostartDir, "%s/qemu/networks/autostart", - base) == -1) - goto out_of_memory; - - VIR_FREE(base); - if (!(driverState->iptables = iptablesContextNew())) { goto out_of_memory; } @@ -418,7 +429,7 @@ networkStateInitialize(bool privileged, driverState->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); if (virNetworkLoadAllState(&driverState->networks, - NETWORK_STATE_DIR) < 0) + driverState->stateDir) < 0) goto error; if (virNetworkLoadAllConfigs(&driverState->networks, @@ -459,18 +470,19 @@ networkStateInitialize(bool privileged, } #endif - return 0; + ret = 0; +cleanup: + VIR_FREE(configdir); + VIR_FREE(rundir); + return ret; out_of_memory: virReportOOMError(); - error: if (driverState) networkDriverUnlock(driverState); - - VIR_FREE(base); networkStateCleanup(); - return -1; + goto cleanup; } /** @@ -486,7 +498,7 @@ networkStateReload(void) { networkDriverLock(driverState); virNetworkLoadAllState(&driverState->networks, - NETWORK_STATE_DIR); + driverState->stateDir); virNetworkLoadAllConfigs(&driverState->networks, driverState->networkConfigDir, driverState->networkAutostartDir); @@ -513,9 +525,12 @@ networkStateCleanup(void) { /* free inactive networks */ virNetworkObjListFree(&driverState->networks); - VIR_FREE(driverState->logDir); VIR_FREE(driverState->networkConfigDir); VIR_FREE(driverState->networkAutostartDir); + VIR_FREE(driverState->stateDir); + VIR_FREE(driverState->pidDir); + VIR_FREE(driverState->dnsmasqStateDir); + VIR_FREE(driverState->radvdStateDir); if (driverState->iptables) iptablesContextFree(driverState->iptables); @@ -1057,32 +1072,33 @@ networkStartDhcpDaemon(struct network_driver *driver, goto cleanup; } - if (virFileMakePath(NETWORK_PID_DIR) < 0) { + if (virFileMakePath(driverState->pidDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), - NETWORK_PID_DIR); + driverState->pidDir); goto cleanup; } - if (virFileMakePath(NETWORK_STATE_DIR) < 0) { + if (virFileMakePath(driverState->stateDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), - NETWORK_STATE_DIR); + driverState->stateDir); goto cleanup; } - if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, network->def->name))) { + if (!(pidfile = virPidFileBuildPath(driverState->pidDir, + network->def->name))) { virReportOOMError(); goto cleanup; } - if (virFileMakePath(DNSMASQ_STATE_DIR) < 0) { + if (virFileMakePath(driverState->dnsmasqStateDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), - DNSMASQ_STATE_DIR); + driverState->dnsmasqStateDir); goto cleanup; } - dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); + dctx = dnsmasqContextNew(network->def->name, driverState->dnsmasqStateDir); if (dctx == NULL) goto cleanup; @@ -1110,7 +1126,7 @@ networkStartDhcpDaemon(struct network_driver *driver, * pid */ - ret = virPidFileRead(NETWORK_PID_DIR, network->def->name, + ret = virPidFileRead(driverState->pidDir, network->def->name, &network->dnsmasqPid); if (ret < 0) goto cleanup; @@ -1147,8 +1163,10 @@ networkRefreshDhcpDaemon(struct network_driver *driver, return networkStartDhcpDaemon(driver, network); VIR_INFO("Refreshing dnsmasq for network %s", network->def->bridge); - if (!(dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR))) + if (!(dctx = dnsmasqContextNew(network->def->name, + driverState->dnsmasqStateDir))) { goto cleanup; + } /* Look for first IPv4 address that has dhcp defined. * We only support dhcp-host config on one IPv4 subnetwork @@ -1372,16 +1390,16 @@ networkStartRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, goto cleanup; } - if (virFileMakePath(NETWORK_PID_DIR) < 0) { + if (virFileMakePath(driverState->pidDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), - NETWORK_PID_DIR); + driverState->pidDir); goto cleanup; } - if (virFileMakePath(RADVD_STATE_DIR) < 0) { + if (virFileMakePath(driverState->radvdStateDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), - RADVD_STATE_DIR); + driverState->radvdStateDir); goto cleanup; } @@ -1390,7 +1408,7 @@ networkStartRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, virReportOOMError(); goto cleanup; } - if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, radvdpidbase))) { + if (!(pidfile = virPidFileBuildPath(driverState->pidDir, radvdpidbase))) { virReportOOMError(); goto cleanup; } @@ -1418,7 +1436,7 @@ networkStartRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase, &network->radvdPid) < 0) + if (virPidFileRead(driverState->pidDir, radvdpidbase, &network->radvdPid) < 0) goto cleanup; ret = 0; @@ -1445,7 +1463,7 @@ networkRefreshRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, network->def->name) >= 0) && ((radvdpidbase = networkRadvdPidfileBasename(network->def->name)) != NULL)) { - virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); + virPidFileDelete(driverState->pidDir, radvdpidbase); VIR_FREE(radvdpidbase); } network->radvdPid = -1; @@ -1485,7 +1503,7 @@ networkRestartRadvd(struct network_driver *driver, network->def->name) >= 0) && ((radvdpidbase = networkRadvdPidfileBasename(network->def->name)) != NULL)) { - virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); + virPidFileDelete(driverState->pidDir, radvdpidbase); VIR_FREE(radvdpidbase); } network->radvdPid = -1; @@ -2575,7 +2593,7 @@ static int networkShutdownNetworkVirtual(struct network_driver *driver, if (!(radvdpidbase = networkRadvdPidfileBasename(network->def->name))) { virReportOOMError(); } else { - virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); + virPidFileDelete(driverState->pidDir, radvdpidbase); VIR_FREE(radvdpidbase); } } @@ -2676,7 +2694,8 @@ networkStartNetwork(struct network_driver *driver, /* Persist the live configuration now that anything autogenerated * is setup. */ - if ((ret = virNetworkSaveStatus(NETWORK_STATE_DIR, network)) < 0) { + if ((ret = virNetworkSaveStatus(driverState->stateDir, + network)) < 0) { goto error; } @@ -2706,7 +2725,8 @@ static int networkShutdownNetwork(struct network_driver *driver, if (!virNetworkObjIsActive(network)) return 0; - stateFile = virNetworkConfigFile(NETWORK_STATE_DIR, network->def->name); + stateFile = virNetworkConfigFile(driverState->stateDir, + network->def->name); if (!stateFile) return -1; @@ -3371,8 +3391,10 @@ networkUpdate(virNetworkPtr net, } /* save current network state to disk */ - if ((ret = virNetworkSaveStatus(NETWORK_STATE_DIR, network)) < 0) + if ((ret = virNetworkSaveStatus(driverState->stateDir, + network)) < 0) { goto cleanup; + } } ret = 0; cleanup: @@ -4705,7 +4727,7 @@ networkPlugBandwidth(virNetworkObjPtr net, /* update sum of 'floor'-s of attached NICs */ net->floor_sum += ifaceBand->in->floor; /* update status file */ - if (virNetworkSaveStatus(NETWORK_STATE_DIR, net) < 0) { + if (virNetworkSaveStatus(driverState->stateDir, net) < 0) { ignore_value(virBitmapClearBit(net->class_id, class_id)); net->floor_sum -= ifaceBand->in->floor; iface->data.network.actual->class_id = 0; @@ -4751,7 +4773,7 @@ networkUnplugBandwidth(virNetworkObjPtr net, ignore_value(virBitmapClearBit(net->class_id, iface->data.network.actual->class_id)); /* update status file */ - if (virNetworkSaveStatus(NETWORK_STATE_DIR, net) < 0) { + if (virNetworkSaveStatus(driverState->stateDir, net) < 0) { net->floor_sum += ifaceBand->in->floor; ignore_value(virBitmapSetBit(net->class_id, iface->data.network.actual->class_id)); -- 1.7.11.7