On 05/02/2013 12:06 PM, 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. > > 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. > --- > src/network/bridge_driver.c | 155 ++++++++++++++++++++++++++++---------------- > 1 file changed, 100 insertions(+), 55 deletions(-) > > > -#define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" So previously, this constant represented the network runtime directory relative to the configured state dir (default /var in a distro installation)... > -#define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" > +#define NETWORK_PID_DIR "/run/libvirt/network" Now it is just the relative suffix of an (as-yet) unspecified prefix... > @@ -84,6 +83,10 @@ struct network_driver { > iptablesContext *iptables; > char *networkConfigDir; > char *networkAutostartDir; > + char *stateDir; > + char *pidDir; > + char *dnsmasqStateDir; > + char *radvdStateDir; ...where the appropriate prefix is computed at initialization and stored for later use... > char *logDir; > dnsmasqCapsPtr dnsmasqCaps; > }; > @@ -133,8 +136,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)); ...all uses now use the computed value instead of the hard-coded macro... > @@ -374,27 +380,59 @@ networkStateInitialize(bool privileged, > networkDriverLock(driverState); > > if (privileged) { > - if (virAsprintf(&driverState->logDir, > - "%s/log/libvirt/qemu", LOCALSTATEDIR) == -1) > - goto out_of_memory; > - > - if ((base = strdup(SYSCONFDIR "/libvirt")) == NULL) > + if (((base = strdup(SYSCONFDIR "/libvirt")) == NULL) || > + ((driverState->logDir > + = strdup(LOCALSTATEDIR "/log/libvirt/qemu")) == NULL) || > + ((driverState->stateDir > + = strdup(LOCALSTATEDIR NETWORK_STATE_DIR)) == NULL) || ...privileged initialization uses LOCALSTATEDIR as its location where the now-relative NETWORK_STATE_DIR is placed,...[1] > + ((driverState->pidDir > + = strdup(LOCALSTATEDIR NETWORK_PID_DIR)) == NULL) || > + ((driverState->dnsmasqStateDir > + = strdup(LOCALSTATEDIR DNSMASQ_STATE_DIR)) == NULL) || > + ((driverState->radvdStateDir > + = strdup(LOCALSTATEDIR RADVD_STATE_DIR)) == NULL)) { > goto out_of_memory; > + } > } else { > char *userdir = virGetUserCacheDirectory(); userdir is now malloc'd...[2] > > if (!userdir) > goto error; > > + userdir = virGetUserConfigDirectory(); [2]...but this overwrites it. Oops. > + if (virAsprintf(&base, "%s", userdir) < 0) { > + VIR_FREE(userdir); > + goto out_of_memory; > + } > + VIR_FREE(userdir); Huh? Simplify this to: base = virGetUserConfigDirectory(); no need to waste a malloc on userdir, just to re-malloc an identical copy of it into base via a beefy virAsprintf call. > + > if (virAsprintf(&driverState->logDir, > - "%s/qemu/log", userdir) == -1) { > + "%s/qemu/log", userdir) < 0) { Huh? Use-after-free of userdir. Did you mean base? > VIR_FREE(userdir); > goto out_of_memory; > } > VIR_FREE(userdir); > > - userdir = virGetUserConfigDirectory(); > - if (virAsprintf(&base, "%s", userdir) == -1) { > + if (!(userdir = virGetUserRuntimeDirectory())) > + goto error; > + > + if (virAsprintf(&driverState->stateDir, > + "%s" NETWORK_STATE_DIR, userdir) < 0) { [2]...for non-privileged code, we are now using the directory relative to the user directory. The idea makes sense, but your implementation of the idea on the qemu:///session side of things needs work. > + if (virAsprintf(&driverState->pidDir, > + "%s" NETWORK_PID_DIR, userdir) < 0) { > + VIR_FREE(userdir); > + goto out_of_memory; > + } > + if (virAsprintf(&driverState->dnsmasqStateDir, > + "%s" DNSMASQ_STATE_DIR, userdir) < 0) { > + VIR_FREE(userdir); > + goto out_of_memory; > + } > + if (virAsprintf(&driverState->radvdStateDir, > + "%s" RADVD_STATE_DIR, userdir) < 0) { > VIR_FREE(userdir); > goto out_of_memory; That's a lot of VIR_FREE(userdir) calls; can you just pre-initialize it to NULL and just blindly move the VIR_FREE(userdir) into the out_of_memory label? Or even chain the conditionals: if (virAsprintf(&driverState->pidDir, "%s" NETWORK_PID_DIR, userdir) < 0 || virAsprintf(&driverState->dnsmasqStateDir, "%s" DNSMASQ_STATE_DIR, userdir) < 0 || ... Looking forward to v2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list