[libvirt] [PATCH 0/4] qemu: Unify cleanup paths in qemuStartVMDaemon
Many of the cleanup paths in qemuStartVMDaemon have the potential of squashing a legitimate startup error message. qemuShutdownVMDaemon already properly handles this, so let's unify all the cleanup to use it. This series fixes a few teardown issues, and changes ShutdownVMDaemon to not reject non-running VMs. This could be an issue if some caller was depending on this functionality, but I couldn't find one. Cole Robinson (4): qemu: Properly cleanup in security startup error path qemu: Remove explicit VNC XML cleanup qemu: Don't deny ShutdownVMDaemon for non-running VMs qemu: Use ShutdownVMDaemon for all startup cleanup paths src/qemu/qemu_driver.c | 31 +++++-------------------------- 1 files changed, 5 insertions(+), 26 deletions(-)
Everything after hostdev setup needs to jump to cleanup on error. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 582fdee..80f96b7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3295,7 +3295,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (driver->securityDriver && driver->securityDriver->domainGenSecurityLabel && driver->securityDriver->domainGenSecurityLabel(vm) < 0) - return -1; + goto cleanup; DEBUG0("Generating setting domain security labels (if required)"); if (driver->securityDriver && -- 1.6.6.1
On 05/17/2010 11:17 AM, Cole Robinson wrote:
Everything after hostdev setup needs to jump to cleanup on error.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 582fdee..80f96b7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3295,7 +3295,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (driver->securityDriver && driver->securityDriver->domainGenSecurityLabel && driver->securityDriver->domainGenSecurityLabel(vm) < 0) - return -1; + goto cleanup;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This only exists for a certain cleanup path in StartVMDaemon, but is unneeded since domain_conf.c handles this for us automatically. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80f96b7..61aacfb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3581,10 +3581,6 @@ cleanup: driver->securityDriver->domainReleaseSecurityLabel) driver->securityDriver->domainReleaseSecurityLabel(vm); qemuRemoveCgroup(driver, vm, 1); - if ((vm->def->ngraphics == 1) && - vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && - vm->def->graphics[0]->data.vnc.autoport) - vm->def->graphics[0]->data.vnc.port = -1; if (logfile != -1) close(logfile); vm->def->id = -1; -- 1.6.6.1
On 05/17/2010 11:17 AM, Cole Robinson wrote:
This only exists for a certain cleanup path in StartVMDaemon, but is unneeded since domain_conf.c handles this for us automatically.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80f96b7..61aacfb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3581,10 +3581,6 @@ cleanup: driver->securityDriver->domainReleaseSecurityLabel) driver->securityDriver->domainReleaseSecurityLabel(vm); qemuRemoveCgroup(driver, vm, 1); - if ((vm->def->ngraphics == 1) && - vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && - vm->def->graphics[0]->data.vnc.autoport) - vm->def->graphics[0]->data.vnc.port = -1; if (logfile != -1) close(logfile); vm->def->id = -1;
ACK, after I read virDomainGraphicsDefParseXML to verify your claim. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Clients that require this already seem to do so. Calling this function with pid < 1 also should not cause problems. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 61aacfb..d2f1a36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3608,10 +3608,8 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainDefPtr def; int i; - if (!virDomainObjIsActive(vm)) - return; - - VIR_DEBUG("Shutting down VM '%s' migrated=%d", vm->def->name, migrated); + VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d", + vm->def->name, vm->pid, migrated); /* This method is routinely used in clean up paths. Disable error * reporting so we don't squash a legit error. */ @@ -3634,6 +3632,7 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, } } + /* This will safely handle a non-running guest with pid=0 or pid=-1*/ if (virKillProcess(vm->pid, 0) == 0 && virKillProcess(vm->pid, SIGTERM) < 0) virReportSystemError(errno, -- 1.6.6.1
On 05/17/2010 11:17 AM, Cole Robinson wrote:
Clients that require this already seem to do so. Calling this function with pid < 1 also should not cause problems.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 61aacfb..d2f1a36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3608,10 +3608,8 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainDefPtr def; int i;
- if (!virDomainObjIsActive(vm)) - return; - - VIR_DEBUG("Shutting down VM '%s' migrated=%d", vm->def->name, migrated); + VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d", + vm->def->name, vm->pid, migrated);
/* This method is routinely used in clean up paths. Disable error * reporting so we don't squash a legit error. */ @@ -3634,6 +3632,7 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, } }
+ /* This will safely handle a non-running guest with pid=0 or pid=-1*/ if (virKillProcess(vm->pid, 0) == 0 && virKillProcess(vm->pid, SIGTERM) < 0) virReportSystemError(errno,
Just comments and a debug statement, so no damage done; and the added comments make the code easier to read. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/18/2010 12:27 PM, Eric Blake wrote:
On 05/17/2010 11:17 AM, Cole Robinson wrote:
Clients that require this already seem to do so. Calling this function with pid < 1 also should not cause problems.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 61aacfb..d2f1a36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3608,10 +3608,8 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, virDomainDefPtr def; int i;
- if (!virDomainObjIsActive(vm)) - return; - - VIR_DEBUG("Shutting down VM '%s' migrated=%d", vm->def->name, migrated); + VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d", + vm->def->name, vm->pid, migrated);
/* This method is routinely used in clean up paths. Disable error * reporting so we don't squash a legit error. */ @@ -3634,6 +3632,7 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, } }
+ /* This will safely handle a non-running guest with pid=0 or pid=-1*/ if (virKillProcess(vm->pid, 0) == 0 && virKillProcess(vm->pid, SIGTERM) < 0) virReportSystemError(errno,
Just comments and a debug statement, so no damage done; and the added comments make the code easier to read.
Dropping the virDomainObjIsActive check is functional change. That's the piece that could cause issues if users depend on it. Doesn't seem to be the case though. - Cole
The current cleanup: in StartVMDaemon path is a poor duplication. qemuShutdownVMDaemon can handle teardown for inactive VMs, so let's use it. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 18 +----------------- 1 files changed, 1 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2f1a36..114917d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3569,23 +3569,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, return 0; cleanup: - /* We jump here if we failed to start the VM for any reason - * XXX investigate if we can kill this block and safely call - * qemudShutdownVMDaemon even though no PID is running */ - qemuDomainReAttachHostDevices(driver, vm->def); - - if (driver->securityDriver && - driver->securityDriver->domainRestoreSecurityAllLabel) - driver->securityDriver->domainRestoreSecurityAllLabel(vm, 0); - if (driver->securityDriver && - driver->securityDriver->domainReleaseSecurityLabel) - driver->securityDriver->domainReleaseSecurityLabel(vm); - qemuRemoveCgroup(driver, vm, 1); - if (logfile != -1) - close(logfile); - vm->def->id = -1; - return -1; - + /* We jump here if we failed to start the VM for any reason */ abort: /* We jump here if we failed to initialize the now running VM * killing it off and pretend we never started it */ -- 1.6.6.1
On 05/17/2010 11:17 AM, Cole Robinson wrote:
The current cleanup: in StartVMDaemon path is a poor duplication. qemuShutdownVMDaemon can handle teardown for inactive VMs, so let's use it.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 18 +----------------- 1 files changed, 1 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2f1a36..114917d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3569,23 +3569,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, return 0;
cleanup: - /* We jump here if we failed to start the VM for any reason - * XXX investigate if we can kill this block and safely call - * qemudShutdownVMDaemon even though no PID is running */ - qemuDomainReAttachHostDevices(driver, vm->def); - - if (driver->securityDriver && - driver->securityDriver->domainRestoreSecurityAllLabel) - driver->securityDriver->domainRestoreSecurityAllLabel(vm, 0); - if (driver->securityDriver && - driver->securityDriver->domainReleaseSecurityLabel) - driver->securityDriver->domainReleaseSecurityLabel(vm); - qemuRemoveCgroup(driver, vm, 1); - if (logfile != -1) - close(logfile); - vm->def->id = -1; - return -1; - + /* We jump here if we failed to start the VM for any reason */ abort: /* We jump here if we failed to initialize the now running VM * killing it off and pretend we never started it */
Agree with the proposed change being safe, but it is incomplete. Now that cleanup: has no code, but falls through immediately to abort:, I'd rather see these merged into one label. Probably cleanup: is the better name, which means changing all the 'goto abort;'. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/18/2010 12:29 PM, Eric Blake wrote:
On 05/17/2010 11:17 AM, Cole Robinson wrote:
The current cleanup: in StartVMDaemon path is a poor duplication. qemuShutdownVMDaemon can handle teardown for inactive VMs, so let's use it.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 18 +----------------- 1 files changed, 1 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d2f1a36..114917d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3569,23 +3569,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, return 0;
cleanup: - /* We jump here if we failed to start the VM for any reason - * XXX investigate if we can kill this block and safely call - * qemudShutdownVMDaemon even though no PID is running */ - qemuDomainReAttachHostDevices(driver, vm->def); - - if (driver->securityDriver && - driver->securityDriver->domainRestoreSecurityAllLabel) - driver->securityDriver->domainRestoreSecurityAllLabel(vm, 0); - if (driver->securityDriver && - driver->securityDriver->domainReleaseSecurityLabel) - driver->securityDriver->domainReleaseSecurityLabel(vm); - qemuRemoveCgroup(driver, vm, 1); - if (logfile != -1) - close(logfile); - vm->def->id = -1; - return -1; - + /* We jump here if we failed to start the VM for any reason */ abort: /* We jump here if we failed to initialize the now running VM * killing it off and pretend we never started it */
Agree with the proposed change being safe, but it is incomplete. Now that cleanup: has no code, but falls through immediately to abort:, I'd rather see these merged into one label. Probably cleanup: is the better name, which means changing all the 'goto abort;'.
I've pushed the first 3 patches, sending a v2 of this one with your recommended changes. Thanks, Cole
participants (2)
-
Cole Robinson -
Eric Blake