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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130502/25f5d36d/attachment-0001.sig>