[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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!


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]