Re: [libvirt] [PATCH 13/13] Run radvd for virtual networks with IPv6 addresses
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!