[libvirt] [PATCH] daemon: Run virStateCleanup conditionally
https://bugzilla.redhat.com/show_bug.cgi?id=1033061 Currently, initialization of drivers is done in a separate thread. This is done for several reasons: a driver that is initialized may require running event loop, it may take ages to initialize driver (e.g. due to autostarting domains). While the thread is spawn and run, the main() continues its execution. However, if something goes bad, or the event loop is just exited (e.g. due to a --timeout or SIGINT) we try to cleanup all the drivers. So we have two threads running Initialize() and Cleanup() concurrently. This may result in accessing stale pointers - e.g. netcf driver will free() itself in stateCleanup callback, while the init thread may come, open a dummy connection in order to autostart some domains and voilà: do_open() iterates over interface drivers and accesses stale netcf driver. The fix consists in not running stateCleanup if the init thread is still running. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/libvirtd.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index aef1546..49c42ad 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -108,6 +108,8 @@ virNetServerProgramPtr remoteProgram = NULL; virNetServerProgramPtr qemuProgram = NULL; virNetServerProgramPtr lxcProgram = NULL; +volatile bool driversInitialized = false; + enum { VIR_DAEMON_ERR_NONE = 0, VIR_DAEMON_ERR_PIDFILE, @@ -912,6 +914,8 @@ static void daemonRunStateInit(void *opaque) goto cleanup; } + driversInitialized = true; + #ifdef HAVE_DBUS /* Tie the non-priviledged libvirtd to the session/shutdown lifecycle */ if (!virNetServerIsPrivileged(srv)) { @@ -1546,7 +1550,8 @@ cleanup: daemonConfigFree(config); - virStateCleanup(); + if (driversInitialized) + virStateCleanup(); return ret; } -- 1.8.4.4
On Tue, Dec 03, 2013 at 11:39:15AM +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1033061
Currently, initialization of drivers is done in a separate thread. This is done for several reasons: a driver that is initialized may require running event loop, it may take ages to initialize driver (e.g. due to autostarting domains). While the thread is spawn and run, the main() continues its execution. However, if something goes bad, or the event loop is just exited (e.g. due to a --timeout or SIGINT) we try to cleanup all the drivers. So we have two threads running Initialize() and Cleanup() concurrently. This may result in accessing stale pointers - e.g. netcf driver will free() itself in stateCleanup callback, while the init thread may come, open a dummy connection in order to autostart some domains and voilà: do_open() iterates over interface drivers and accesses stale netcf driver.
The fix consists in not running stateCleanup if the init thread is still running.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/libvirtd.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index aef1546..49c42ad 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -108,6 +108,8 @@ virNetServerProgramPtr remoteProgram = NULL; virNetServerProgramPtr qemuProgram = NULL; virNetServerProgramPtr lxcProgram = NULL;
+volatile bool driversInitialized = false; + enum { VIR_DAEMON_ERR_NONE = 0, VIR_DAEMON_ERR_PIDFILE, @@ -912,6 +914,8 @@ static void daemonRunStateInit(void *opaque) goto cleanup; }
+ driversInitialized = true; + #ifdef HAVE_DBUS /* Tie the non-priviledged libvirtd to the session/shutdown lifecycle */ if (!virNetServerIsPrivileged(srv)) { @@ -1546,7 +1550,8 @@ cleanup:
daemonConfigFree(config);
- virStateCleanup(); + if (driversInitialized) + virStateCleanup();
Don't we technically need to use an int and atomic int APIs for these changes ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/03/2013 03:43 AM, Daniel P. Berrange wrote:
On Tue, Dec 03, 2013 at 11:39:15AM +0100, Michal Privoznik wrote:
+ driversInitialized = true; + #ifdef HAVE_DBUS /* Tie the non-priviledged libvirtd to the session/shutdown lifecycle */ if (!virNetServerIsPrivileged(srv)) { @@ -1546,7 +1550,8 @@ cleanup:
daemonConfigFree(config);
- virStateCleanup(); + if (driversInitialized) + virStateCleanup();
Don't we technically need to use an int and atomic int APIs for these changes ?
Not as far as I can tell. Since only one thread is setting the variable, and we are not accessing the variable in a signal handler, it seems that mere 'volatile' is enough to ensure that the compiler won't optimize any out-of-order assignments. I'm fine giving ACK to this patch as-is. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 03, 2013 at 06:41:36AM -0700, Eric Blake wrote:
On 12/03/2013 03:43 AM, Daniel P. Berrange wrote:
On Tue, Dec 03, 2013 at 11:39:15AM +0100, Michal Privoznik wrote:
+ driversInitialized = true; + #ifdef HAVE_DBUS /* Tie the non-priviledged libvirtd to the session/shutdown lifecycle */ if (!virNetServerIsPrivileged(srv)) { @@ -1546,7 +1550,8 @@ cleanup:
daemonConfigFree(config);
- virStateCleanup(); + if (driversInitialized) + virStateCleanup();
Don't we technically need to use an int and atomic int APIs for these changes ?
Not as far as I can tell. Since only one thread is setting the variable, and we are not accessing the variable in a signal handler, it seems that mere 'volatile' is enough to ensure that the compiler won't optimize any out-of-order assignments. I'm fine giving ACK to this patch as-is.
I wasn't thinking of ordering, but rather atomicity of updating the value. eg what happens if thread 1 tries to read the value concurrently with thread 2 setting it. Surely this is the case atomic ops are intended for. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/03/2013 06:55 AM, Daniel P. Berrange wrote:
Not as far as I can tell. Since only one thread is setting the variable, and we are not accessing the variable in a signal handler, it seems that mere 'volatile' is enough to ensure that the compiler won't optimize any out-of-order assignments. I'm fine giving ACK to this patch as-is.
I wasn't thinking of ordering, but rather atomicity of updating the value. eg what happens if thread 1 tries to read the value concurrently with thread 2 setting it. Surely this is the case atomic ops are intended for.
Still no problem. You're thinking of two threads trying to write a variable, where there is a race on which value gets written. But with one thread writing exactly once, and the other thread reading, you get exactly one of two well-defined behaviors: The reader is first, which means initialization hasn't yet completed, so we skip cleanup (rare, but the case of the BZ we are trying to fix) The writer is first, which means initialization has completed and the cleanup thread can clean up (the common case). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik