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

2 May 2013


      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

    