[libvirt] [PATCH 13/13] Run radvd for virtual networks with IPv6 addresses

[Eric Blake]

On 12/20/2010 01:03 AM, Laine Stump wrote:
> 
> 1) Don't attempt to immediately read the pidfile and store the pid in
>    memory. Instead, just read the pidfile later when we want to kill
>    radvd. (This could still lead to a race if networkStart and
>    networkDestroy were called in tight sequence by some application)

Still racy unless you go with the extra complexity of using inotify() to
determine when the radvd process has completed writing to the pidfile.
Seems rather complex, but since we're already assuming Linux, it doesn't
seem to hard to assumen inotify().

> 
> 2) Don't allow radvd to daemonize itself. Instead, run it with "-d 1"
>    (setting a debuglevel > 0 implies that it should not daemonize),
>    and use virCommand's own pidfile/daemonize functions. (The problem
>    here is that there's no way to tell radvd to not write a pidfile
>    itself, so we would end up with 2 pidfiles for each instance. This
>    isn't a functional problem, just cosmetic).

Looks like you can use radvd -p /dev/null to make radvd avoid the second
pidfile, and just go with virCommand's pidfile.  How much extra output
does radvd -d 1 cause in comparison to radvd -d 0?

I'm leaning towards option 2; it's better to avoid the race.

> 
> Another unresolved(?) problem is that the location of the radvd binary
> is found by autoconf during the build, so if it's not present on the
> build machine. Should this be handled by specfiles on specific distros
> adding a BuildRequires for the radvd package, or can/should more be
> done in the libvirt tree? (Current behavior is identical to dnsmasq,
> so I guess it's acceptable, as long as all the downstream builders are
> made aware of the need for radvd for proper IPv6 support).

specfiles sound like the way to go to me.

> +static char *
> +networkRadvdConfigFileName(const char *netname)
> +{
> +    char *configfile;
> +
> +    virAsprintf(&configfile, "%s/%s-radvd.conf",
> +                RADVD_STATE_DIR, netname);

It's slightly more efficient to do
virAsprintf(&configfile, RADVD_STATE_DIR "/%s-radvd.conf",
            netname);

but it doesn't hurt my feelings to leave it as is.

> +            /* Try and read dnsmasq/radvd pids if any */
> +            if (obj->def->ips && (obj->def->nips > 0)) {
> +                char *pidpath, *radvdpidbase;
> +
> +                if (virFileReadPid(NETWORK_PID_DIR, obj->def->name,
> +                                   &obj->dnsmasqPid) == 0) {

Thinking out loud here - can virFileReadPid be taught how to inspect
whether other processes still have the pidfile open for writing, and
block until those other files have closed?

Another thought - abuse fcntl(F_SETLK), to pass the radvd process an
open and locked fd pointing to the same pid file that the radvd process
will separately open.  When the radvd process then close()s the pidfile,
that will nuke the fcntl lock, and the parent process can use that to
tell when things are done.  Unfortunately, reading the man page further,
this doesn't sound too good (that is, inotify() is more robust), because
fcntl locks are not preserved across fork(), but radvd calls daemon()
which calls fork(), so the lock would be lost before the daemon process
starts.

> +
> +    cmd = virCommandNew(RADVD);
> +    virCommandAddArgList(cmd, "--config", configfile,
> +                         "--pidfile", pidfile,
> +                         NULL);

You could combine these:

cmd = virCommandNewArgList(RADVD, "--config", configfile,
            "--pidfile", pidfile, NULL);

>  
> +    if (v6present) {
> +        char *configfile = networkRadvdConfigFileName(network->def->name);
> +
> +        if (!configfile) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +        unlink(configfile);
> +    }

Where do you VIR_FREE(configfile)?

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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