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! -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list