On Fri, May 03, 2013 at 08:24:14AM -0400, Laine Stump wrote: > 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 Sorry for the delay. Scratch build of libvirt 1.0.5 + this patch: http://koji.fedoraproject.org/koji/taskinfo?taskID=5328375 I have tested this and it fixes the issue I was seeing in the bug report, and doesn't appear to break libguestfs. Thus, ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list