1:22 p.m.
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