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

[Laine Stump]

On 12/20/2010 06:22 PM, Eric Blake wrote:
> 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?

Very little.

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

And it works! I've folded that into the v2 patch.

>> +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.

Changed anyway.


>
>> +
>> +    cmd = virCommandNew(RADVD);
>> +    virCommandAddArgList(cmd, "--config", configfile,
>> +                         "--pidfile", pidfile,
>> +                         NULL);
> You could combine these:
>
> cmd = virCommandNewArgList(RADVD, "--config", configfile,
>              "--pidfile", pidfile, NULL);

Nice! I didn't see that function.

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

(eep) Er, I didn't :-/ Now I do. Thanks for catching that!