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.