From: Luyao Huang <lhuang@redhat.com> https://bugzilla.redhat.com/show_bug.cgi?id=1176503 Move the two console checks - one for zero nconsoles present and the other for an invalid console type to earlier in the processing rather than getting after performing some setup that has to be undone for what amounts to an invalid configuration. This resolves the above bug since it's not not possible to have changed the security labels when we cause the configuration check failure. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_process.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 19ea7f3..1a26a70 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1050,6 +1050,20 @@ int virLXCProcessStart(virConnectPtr conn, } virCgroupFree(&selfcgroup); + if (vm->def->nconsoles == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("At least one PTY console is required")); + return -1; + } + + for (i = 0; i < vm->def->nconsoles; i++) { + if (vm->def->consoles[i]->source.type != VIR_DOMAIN_CHR_TYPE_PTY) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only PTY console types are supported")); + return -1; + } + } + if (virFileMakePath(cfg->logDir) < 0) { virReportSystemError(errno, _("Cannot create log directory '%s'"), @@ -1146,19 +1160,8 @@ int virLXCProcessStart(virConnectPtr conn, vm->def, NULL) < 0) goto cleanup; - if (vm->def->nconsoles == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("At least one PTY console is required")); - goto cleanup; - } - for (i = 0; i < vm->def->nconsoles; i++) { char *ttyPath; - if (vm->def->consoles[i]->source.type != VIR_DOMAIN_CHR_TYPE_PTY) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only PTY console types are supported")); - goto cleanup; - } if (virFileOpenTty(&ttyFDs[i], &ttyPath, 1) < 0) { virReportSystemError(errno, "%s", -- 2.1.0

Modify the VIR_DEBUG message in virLXCProcessCleanup to make it clearer about the path. Also add some more VIR_DEBUG messages in virLXCProcessStart in order to help debug error flow. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_process.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 1a26a70..4d9bf67 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -154,7 +154,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, virNetDevVPortProfilePtr vport = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); - VIR_DEBUG("Stopping VM name=%s pid=%d reason=%d", + VIR_DEBUG("Cleanup VM name=%s pid=%d reason=%d", vm->def->name, (int)vm->pid, (int)reason); /* now that we know it's stopped call the hook if present */ @@ -1160,6 +1160,7 @@ int virLXCProcessStart(virConnectPtr conn, vm->def, NULL) < 0) goto cleanup; + VIR_DEBUG("Setting up consoles"); for (i = 0; i < vm->def->nconsoles; i++) { char *ttyPath; @@ -1177,9 +1178,11 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; } + VIR_DEBUG("Setting up Interfaces"); if (virLXCProcessSetupInterfaces(conn, vm->def, &nveths, &veths) < 0) goto cleanup; + VIR_DEBUG("Preparing to launch"); if ((logfd = open(logfile, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR|S_IWUSR)) < 0) { virReportSystemError(errno, @@ -1237,6 +1240,7 @@ int virLXCProcessStart(virConnectPtr conn, VIR_WARN("Unable to seek to end of logfile: %s", virStrerror(errno, ebuf, sizeof(ebuf))); + VIR_DEBUG("Launching container"); virCommandRawStatus(cmd); if (virCommandRun(cmd, &status) < 0) goto cleanup; -- 2.1.0

From: Luyao Huang <lhuang@redhat.com> Jumping to the cleanup label prior to starting the container failed to properly clean everything up that is handled by the virLXCProcessCleanup which is called if virLXCProcessStop is called on failure after the container properly starts. Most importantly is prior to this patch none of the stop/release hooks, host device reattachment, and network cleanup (that is reverse of virLXCProcessSetupInterfaces). Signed-off-by: Luyao Huang <lhuang@redhat.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_process.c | 78 ++++++++++++++++++++++----------------------------- 1 file changed, 33 insertions(+), 45 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 4d9bf67..a3410e4 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1023,6 +1023,7 @@ int virLXCProcessStart(virConnectPtr conn, int status; char *pidfile = NULL; bool clearSeclabel = false; + bool need_stop = false; if (virCgroupNewSelf(&selfcgroup) < 0) return -1; @@ -1271,6 +1272,7 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; } + need_stop = true; priv->stopReason = VIR_DOMAIN_EVENT_STOPPED_FAILED; priv->wantReboot = false; vm->def->id = vm->pid; @@ -1279,20 +1281,20 @@ int virLXCProcessStart(virConnectPtr conn, if (VIR_CLOSE(handshakefds[1]) < 0) { virReportSystemError(errno, "%s", _("could not close handshake fd")); - goto error; + goto cleanup; } if (virCommandHandshakeWait(cmd) < 0) - goto error; + goto cleanup; /* Write domain status to disk for the controller to * read when it starts */ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto error; + goto cleanup; /* Allow the child to exec the controller */ if (virCommandHandshakeNotify(cmd) < 0) - goto error; + goto cleanup; if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); @@ -1305,7 +1307,7 @@ int virLXCProcessStart(virConnectPtr conn, _("guest failed to start: %s"), out); } - goto error; + goto cleanup; } /* We know the cgroup must exist by this synchronization @@ -1317,13 +1319,13 @@ int virLXCProcessStart(virConnectPtr conn, vm->def->resource->partition : NULL, -1, &priv->cgroup) < 0) - goto error; + goto cleanup; if (!priv->cgroup) { virReportError(VIR_ERR_INTERNAL_ERROR, _("No valid cgroup for machine %s"), vm->def->name); - goto error; + goto cleanup; } /* And we can get the first monitor connection now too */ @@ -1336,17 +1338,17 @@ int virLXCProcessStart(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("guest failed to start: %s"), ebuf); } - goto error; + goto cleanup; } if (autoDestroy && virCloseCallbacksSet(driver->closeCallbacks, vm, conn, lxcProcessAutoDestroy) < 0) - goto error; + goto cleanup; if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, false) < 0) - goto error; + goto cleanup; /* We don't need the temporary NIC names anymore, clear them */ virLXCProcessCleanInterfaces(vm->def); @@ -1365,48 +1367,39 @@ int virLXCProcessStart(virConnectPtr conn, * If the script raised an error abort the launch */ if (hookret < 0) - goto error; + goto cleanup; } rc = 0; cleanup: - if (rc != 0 && !err) - err = virSaveLastError(); - virCommandFree(cmd); if (VIR_CLOSE(logfd) < 0) { virReportSystemError(errno, "%s", _("could not close logfile")); rc = -1; } - for (i = 0; i < nveths; i++) { - if (rc != 0 && veths[i]) - ignore_value(virNetDevVethDelete(veths[i])); - VIR_FREE(veths[i]); - } if (rc != 0) { - if (vm->newDef) { - virDomainDefFree(vm->newDef); - vm->newDef = NULL; - } - if (priv->monitor) { - virObjectUnref(priv->monitor); - priv->monitor = NULL; - } - virDomainConfVMNWFilterTeardown(vm); - - virSecurityManagerRestoreAllLabel(driver->securityManager, - vm->def, false); - virSecurityManagerReleaseLabel(driver->securityManager, vm->def); - /* Clear out dynamically assigned labels */ - if (vm->def->nseclabels && - (vm->def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DYNAMIC || - clearSeclabel)) { - VIR_FREE(vm->def->seclabels[0]->model); - VIR_FREE(vm->def->seclabels[0]->label); - VIR_FREE(vm->def->seclabels[0]->imagelabel); - VIR_DELETE_ELEMENT(vm->def->seclabels, 0, vm->def->nseclabels); + err = virSaveLastError(); + if (need_stop) { + virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); + } else { + virSecurityManagerRestoreAllLabel(driver->securityManager, + vm->def, false); + virSecurityManagerReleaseLabel(driver->securityManager, vm->def); + /* Clear out dynamically assigned labels */ + if (vm->def->nseclabels && + (vm->def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DYNAMIC || + clearSeclabel)) { + VIR_FREE(vm->def->seclabels[0]->model); + VIR_FREE(vm->def->seclabels[0]->label); + VIR_FREE(vm->def->seclabels[0]->imagelabel); + VIR_DELETE_ELEMENT(vm->def->seclabels, 0, vm->def->nseclabels); + } + virLXCProcessCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); } } + virCommandFree(cmd); + for (i = 0; i < nveths; i++) + VIR_FREE(veths[i]); for (i = 0; i < nttyFDs; i++) VIR_FORCE_CLOSE(ttyFDs[i]); VIR_FREE(ttyFDs); @@ -1423,11 +1416,6 @@ int virLXCProcessStart(virConnectPtr conn, } return rc; - - error: - err = virSaveLastError(); - virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); - goto cleanup; } struct virLXCProcessAutostartData { -- 2.1.0