[libvirt] [PATCH] Fix cleanup on VM state after failed QEMU startup
Commit 9962e406c664ed5521f5aca500c860a331cb3979 introduced a problem where if the VM failed to startup, it would not be correctly cleaned up. Amongst other things the SELinux security label would not be removed, which prevents the VM from ever starting again. The virDomainIsActive() check at the start of qemudShutdownVMDaemon checks for vm->def->id not being -1. By moving the assignment of the VM id to the start of qemudStartVMDaemon, we can ensure cleanup will occur on failure * src/qemu/qemu_driver.c: Move initialization of 'vm->def->id' so that qemudShutdownVMDaemon() will process the shutdown --- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 21d7779..d7c806b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2652,6 +2652,8 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) goto cleanup; + vm->def->id = driver->nextvmid++; + /* Must be run before security labelling */ DEBUG0("Preparing host devices"); if (qemuPrepareHostDevices(driver, vm->def) < 0) @@ -2818,7 +2820,6 @@ static int qemudStartVMDaemon(virConnectPtr conn, } DEBUG0("Building emulator command line"); - vm->def->id = driver->nextvmid++; if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, qemuCmdFlags, migrateFrom, stdin_fd, -- 1.7.4
At 02/14/2011 05:38 PM, Daniel P. Berrange Write:
Commit 9962e406c664ed5521f5aca500c860a331cb3979 introduced a problem where if the VM failed to startup, it would not be correctly cleaned up. Amongst other things the SELinux security label would not be removed, which prevents the VM from ever starting again.
The virDomainIsActive() check at the start of qemudShutdownVMDaemon checks for vm->def->id not being -1. By moving the assignment of the VM id to the start of qemudStartVMDaemon, we can ensure cleanup will occur on failure
* src/qemu/qemu_driver.c: Move initialization of 'vm->def->id' so that qemudShutdownVMDaemon() will process the shutdown --- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 21d7779..d7c806b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2652,6 +2652,8 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) goto cleanup;
+ vm->def->id = driver->nextvmid++; +
The function virDomainObjSetDefTransient() does not need cleanup? I think we can init 'vm->def->id' after we check if the domain is started.
/* Must be run before security labelling */ DEBUG0("Preparing host devices"); if (qemuPrepareHostDevices(driver, vm->def) < 0) @@ -2818,7 +2820,6 @@ static int qemudStartVMDaemon(virConnectPtr conn, }
DEBUG0("Building emulator command line"); - vm->def->id = driver->nextvmid++; if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, qemuCmdFlags, migrateFrom, stdin_fd,
On Mon, Feb 14, 2011 at 06:01:49PM +0800, Wen Congyang wrote:
At 02/14/2011 05:38 PM, Daniel P. Berrange Write:
Commit 9962e406c664ed5521f5aca500c860a331cb3979 introduced a problem where if the VM failed to startup, it would not be correctly cleaned up. Amongst other things the SELinux security label would not be removed, which prevents the VM from ever starting again.
The virDomainIsActive() check at the start of qemudShutdownVMDaemon checks for vm->def->id not being -1. By moving the assignment of the VM id to the start of qemudStartVMDaemon, we can ensure cleanup will occur on failure
* src/qemu/qemu_driver.c: Move initialization of 'vm->def->id' so that qemudShutdownVMDaemon() will process the shutdown --- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 21d7779..d7c806b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2652,6 +2652,8 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) goto cleanup;
+ vm->def->id = driver->nextvmid++; +
The function virDomainObjSetDefTransient() does not need cleanup?
Only if it is successful, which is what this does.
I think we can init 'vm->def->id' after we check if the domain is started.
Nope, we need to be able to reverse the results of every function run in qemudStartVMDaemon, regardless of whether we actually get as far as the virCommandRun step.
/* Must be run before security labelling */ DEBUG0("Preparing host devices"); if (qemuPrepareHostDevices(driver, vm->def) < 0) @@ -2818,7 +2820,6 @@ static int qemudStartVMDaemon(virConnectPtr conn, }
DEBUG0("Building emulator command line"); - vm->def->id = driver->nextvmid++; if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, qemuCmdFlags, migrateFrom, stdin_fd,
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 Mon, Feb 14, 2011 at 09:38:37AM +0000, Daniel P. Berrange wrote:
Commit 9962e406c664ed5521f5aca500c860a331cb3979 introduced a problem where if the VM failed to startup, it would not be correctly cleaned up. Amongst other things the SELinux security label would not be removed, which prevents the VM from ever starting again.
The virDomainIsActive() check at the start of qemudShutdownVMDaemon checks for vm->def->id not being -1. By moving the assignment of the VM id to the start of qemudStartVMDaemon, we can ensure cleanup will occur on failure
* src/qemu/qemu_driver.c: Move initialization of 'vm->def->id' so that qemudShutdownVMDaemon() will process the shutdown --- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 21d7779..d7c806b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2652,6 +2652,8 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (virDomainObjSetDefTransient(driver->caps, vm, true) < 0) goto cleanup;
+ vm->def->id = driver->nextvmid++; + /* Must be run before security labelling */ DEBUG0("Preparing host devices"); if (qemuPrepareHostDevices(driver, vm->def) < 0) @@ -2818,7 +2820,6 @@ static int qemudStartVMDaemon(virConnectPtr conn, }
DEBUG0("Building emulator command line"); - vm->def->id = driver->nextvmid++; if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, qemuCmdFlags, migrateFrom, stdin_fd,
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Wen Congyang