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

[Laine Stump]

On 12/23/2010 12:54 PM, Eric Blake wrote:
> On 12/22/2010 11:58 AM, Laine Stump wrote:
>> Running an instance of the router advertisement daemon (radvd) allows
>> guests using the virtual network to automatically acquire an IPv6
>> address and default route. Note that acquiring an address only works
>> for networks with a prefix length of exactly 64 - radvd is still run
>> in other circumstances, and still advertises routes, but autoconf will
>> not work because it requires exactly 64 bits of address info from the
>> network prefix.
>>
>> This patch avoids a race condition with the pidfile by manually
>> daemonizing radvd rather than allowing it to daemonize itself, then
>> creating our own pidfile (in addition to radvd's own file, which is
>> unnecessary). This is accomplished by exec'ing it with "--debug 1" in
>> the commandline, and using virCommand's features to fork, create a
>> pidfile, and detach from the newly forked process.
> As a reminder, have you filed a bug report against radvd about their
> anti-social daemonizing behavior?

Yes, I filed two. One about the race, and one about the inability to 
specify "no pidfile". The "no pidfile" one has already been closed as 
NOTABUG. The one about the race is still NEW.

https://bugzilla.redhat.com/show_bug.cgi?id=664791
https://bugzilla.redhat.com/show_bug.cgi?id=664783

(I filed them against RHEL6, because I'm not sure where upstream is, but 
am sure that the RHEL maintainer does).
>> +    cmd = virCommandNewArgList(RADVD, "--debug", "1",
>> +                               "--config", configfile,
>> +                               "--pidfile", NULL);
>> +    virCommandAddArgFormat(cmd, "%s-bin", pidfile);
>> +
>> +    virCommandSetPidFile(cmd, pidfile);
>> +    virCommandDaemonize(cmd);
>> +
>> +    if (virCommandRun(cmd, NULL)<  0)
>> +        goto cleanup;
>> +
>> +    if (virFileReadPid(NETWORK_PID_DIR, radvdpidbase,
>> +&network->radvdPid)<  0)
>> +        goto cleanup;
>> +
>> +    ret = 0;
>> +cleanup:
>> +    virCommandFree(cmd);
>> +    VIR_FREE(configfile);
>> +    VIR_FREE(configstr);
>> +    virBufferFreeAndReset(&configbuf);
>> +    VIR_FREE(radvdpidbase);
>> +    VIR_FREE(pidfile);
> Should we also unlink() pidfile or pidfile-bin at any point?

we need to unlink pidfile, but radvd seems to guarantee that pidfile-bin 
is unlinked when it exits.

I guess we should do it both when we kill radvd, and also when we 
undefine the network, since there may have been a situation that killed 
radvd without libvirt's knowledge, in which case an extra will be 
hanging around.

I'll squash that in as well.

>>   static int
>> -networkEnableIpForwarding(void)
>> +networkEnableIpForwarding(int enableIPv4, int enableIPv6)
> s/int/bool/

Okay.

> ACK with those nits addressed.
>