Makes it more likely we get useful error output in the logs Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/lxc/lxc_container.c | 29 ++++++++++++++++------------- 1 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 9ae93b5..173af07 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -751,9 +751,9 @@ static int lxcContainerChild( void *data ) { lxc_child_argv_t *argv = data; virDomainDefPtr vmDef = argv->config; - int ttyfd; + int ttyfd = -1; int ret = -1; - char *ttyPath; + char *ttyPath = NULL; virDomainFSDefPtr root; virCommandPtr cmd = NULL; @@ -786,16 +786,8 @@ static int lxcContainerChild( void *data ) virReportSystemError(errno, _("Failed to open tty %s"), ttyPath); - VIR_FREE(ttyPath); goto cleanup; } - VIR_FREE(ttyPath); - - if (lxcContainerSetStdio(argv->monitor, ttyfd) < 0) { - VIR_FORCE_CLOSE(ttyfd); - goto cleanup; - } - VIR_FORCE_CLOSE(ttyfd); if (lxcContainerSetupMounts(vmDef, root) < 0) goto cleanup; @@ -806,17 +798,28 @@ static int lxcContainerChild( void *data ) /* rename and enable interfaces */ if (lxcContainerRenameAndEnableInterfaces(argv->nveths, - argv->veths) < 0) + argv->veths) < 0) { goto cleanup; + } /* drop a set of root capabilities */ if (lxcContainerDropCapabilities() < 0) goto cleanup; - /* this function will only return if an error occured */ - ret = virCommandExec(cmd); + if (lxcContainerSetStdio(argv->monitor, ttyfd) < 0) { + goto cleanup; + } + ret = 0; cleanup: + VIR_FREE(ttyPath); + VIR_FORCE_CLOSE(ttyfd); + + if (ret == 0) { + /* this function will only return if an error occured */ + ret = virCommandExec(cmd); + } + virCommandFree(cmd); return ret; } -- 1.7.4.4

We will reuse these shortly, and each use should have a different error message. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/lxc/lxc_container.c | 15 ++++++--------- src/lxc/lxc_controller.c | 5 ++++- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 173af07..00add94 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -195,13 +195,10 @@ int lxcContainerSendContinue(int control) writeCount = safewrite(control, &msg, sizeof(msg)); if (writeCount != sizeof(msg)) { - virReportSystemError(errno, "%s", - _("Unable to send container continue message")); goto error_out; } rc = 0; - error_out: return rc; } @@ -224,13 +221,8 @@ static int lxcContainerWaitForContinue(int control) readLen = saferead(control, &msg, sizeof(msg)); if (readLen != sizeof(msg) || msg != LXC_CONTINUE_MSG) { - virReportSystemError(errno, "%s", - _("Failed to read the container continue message")); return -1; } - VIR_FORCE_CLOSE(control); - - VIR_DEBUG("Received container continue message"); return 0; } @@ -793,8 +785,12 @@ static int lxcContainerChild( void *data ) goto cleanup; /* Wait for interface devices to show up */ - if (lxcContainerWaitForContinue(argv->monitor) < 0) + if (lxcContainerWaitForContinue(argv->monitor) < 0) { + virReportSystemError(errno, "%s", + _("Failed to read the container continue message")); goto cleanup; + } + VIR_DEBUG("Received container continue message"); /* rename and enable interfaces */ if (lxcContainerRenameAndEnableInterfaces(argv->nveths, @@ -814,6 +810,7 @@ static int lxcContainerChild( void *data ) cleanup: VIR_FREE(ttyPath); VIR_FORCE_CLOSE(ttyfd); + VIR_FORCE_CLOSE(argv->monitor); if (ret == 0) { /* this function will only return if an error occured */ diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 5c4bd1f..cef4b58 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -731,8 +731,11 @@ lxcControllerRun(virDomainDefPtr def, if (lxcControllerMoveInterfaces(nveths, veths, container) < 0) goto cleanup; - if (lxcContainerSendContinue(control[0]) < 0) + if (lxcContainerSendContinue(control[0]) < 0) { + virReportSystemError(errno, "%s", + _("Unable to send container continue message")); goto cleanup; + } /* Now the container is running, there's no need for us to keep any elevated capabilities */ -- 1.7.4.4

Arranges things similar to the qemu driver. Will allow us to more easilly report command error output. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/lxc/lxc_driver.c | 74 ++++++++++++++++++++++++++----------------------- 1 files changed, 39 insertions(+), 35 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8eb87a2..99f94ff 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1281,21 +1281,18 @@ cleanup: } -static int lxcControllerStart(lxc_driver_t *driver, - virDomainObjPtr vm, - int nveths, - char **veths, - int appPty, - int logfile) +static virCommandPtr +lxcBuildControllerCmd(lxc_driver_t *driver, + virDomainObjPtr vm, + int nveths, + char **veths, + int appPty, + int logfile) { int i; - int ret = -1; char *filterstr; char *outputstr; virCommandPtr cmd; - off_t pos = -1; - char ebuf[1024]; - char *timestamp; cmd = virCommandNew(vm->def->emulator); @@ -1357,33 +1354,14 @@ static int lxcControllerStart(lxc_driver_t *driver, goto cleanup; } - /* Log timestamp */ - if ((timestamp = virTimestamp()) == NULL) { - virReportOOMError(); - goto cleanup; - } - if (safewrite(logfile, timestamp, strlen(timestamp)) < 0 || - safewrite(logfile, START_POSTFIX, strlen(START_POSTFIX)) < 0) { - VIR_WARN("Unable to write timestamp to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - } - VIR_FREE(timestamp); - - /* Log generated command line */ - virCommandWriteArgLog(cmd, logfile); - if ((pos = lseek(logfile, 0, SEEK_END)) < 0) - VIR_WARN("Unable to seek to end of logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - virCommandPreserveFD(cmd, appPty); virCommandSetOutputFD(cmd, &logfile); virCommandSetErrorFD(cmd, &logfile); - ret = virCommandRun(cmd, NULL); - + return cmd; cleanup: virCommandFree(cmd); - return ret; + return NULL; } @@ -1411,6 +1389,10 @@ static int lxcVmStart(virConnectPtr conn, int logfd = -1; unsigned int nveths = 0; char **veths = NULL; + off_t pos = -1; + char ebuf[1024]; + char *timestamp; + virCommandPtr cmd = NULL; lxcDomainObjPrivatePtr priv = vm->privateData; if (!lxc_driver->cgroup) { @@ -1480,10 +1462,31 @@ static int lxcVmStart(virConnectPtr conn, goto cleanup; } - if (lxcControllerStart(driver, - vm, - nveths, veths, - parentTty, logfd) < 0) + if (!(cmd = lxcBuildControllerCmd(driver, + vm, + nveths, veths, + parentTty, logfd))) + goto cleanup; + + /* Log timestamp */ + if ((timestamp = virTimestamp()) == NULL) { + virReportOOMError(); + goto cleanup; + } + if (safewrite(logfd, timestamp, strlen(timestamp)) < 0 || + safewrite(logfd, START_POSTFIX, strlen(START_POSTFIX)) < 0) { + VIR_WARN("Unable to write timestamp to logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); + } + VIR_FREE(timestamp); + + /* Log generated command line */ + virCommandWriteArgLog(cmd, logfd); + if ((pos = lseek(logfd, 0, SEEK_END)) < 0) + VIR_WARN("Unable to seek to end of logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); + + if (virCommandRun(cmd, NULL) < 0) goto cleanup; /* Connect to the controller as a client *first* because @@ -1529,6 +1532,7 @@ static int lxcVmStart(virConnectPtr conn, rc = 0; cleanup: + virCommandFree(cmd); if (VIR_CLOSE(logfd) < 0) { virReportSystemError(errno, "%s", _("could not close logfile")); rc = -1; -- 1.7.4.4

Add a simple handshake with the lxc_controller process so we can detect process startup failures. We do this by adding a new --handshake cli arg to lxc_controller for passing a file descriptor. If the process fails to launch, we scrape all output from the logfile and report it to the user. Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_container.h | 1 + src/lxc/lxc_controller.c | 33 +++++++++++++- src/lxc/lxc_driver.c | 107 +++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 137 insertions(+), 6 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 00add94..ff90842 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -213,7 +213,7 @@ error_out: * * Returns 0 on success or -1 in case of error */ -static int lxcContainerWaitForContinue(int control) +int lxcContainerWaitForContinue(int control) { lxc_message_t msg; int readLen; diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h index 5e08d45..a3e457e 100644 --- a/src/lxc/lxc_container.h +++ b/src/lxc/lxc_container.h @@ -46,6 +46,7 @@ enum { # define LXC_DEV_MAJ_PTY 136 int lxcContainerSendContinue(int control); +int lxcContainerWaitForContinue(int control); int lxcContainerStart(virDomainDefPtr def, unsigned int nveths, diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index cef4b58..5bf8ee3 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -612,7 +612,8 @@ lxcControllerRun(virDomainDefPtr def, char **veths, int monitor, int client, - int appPty) + int appPty, + int handshakefd) { int rc = -1; int control[2] = { -1, -1}; @@ -742,6 +743,13 @@ lxcControllerRun(virDomainDefPtr def, if (lxcControllerClearCapabilities() < 0) goto cleanup; + if (lxcContainerSendContinue(handshakefd) < 0) { + virReportSystemError(errno, "%s", + _("error sending continue signal to parent")); + goto cleanup; + } + VIR_FORCE_CLOSE(handshakefd); + rc = lxcControllerMain(monitor, client, appPty, containerPty, container); cleanup: @@ -751,6 +759,7 @@ cleanup: VIR_FORCE_CLOSE(control[1]); VIR_FREE(containerPtyPath); VIR_FORCE_CLOSE(containerPty); + VIR_FORCE_CLOSE(handshakefd); if (container > 1) { int status; @@ -774,6 +783,7 @@ int main(int argc, char *argv[]) char **veths = NULL; int monitor = -1; int appPty = -1; + int handshakefd = -1; int bg = 0; virCapsPtr caps = NULL; virDomainDefPtr def = NULL; @@ -784,6 +794,7 @@ int main(int argc, char *argv[]) { "name", 1, NULL, 'n' }, { "veth", 1, NULL, 'v' }, { "console", 1, NULL, 'c' }, + { "handshakefd", 1, NULL, 's' }, { "help", 0, NULL, 'h' }, { 0, 0, 0, 0 }, }; @@ -798,7 +809,7 @@ int main(int argc, char *argv[]) while (1) { int c; - c = getopt_long(argc, argv, "dn:v:m:c:h", + c = getopt_long(argc, argv, "dn:v:m:c:s:h", options, NULL); if (c == -1) @@ -834,6 +845,14 @@ int main(int argc, char *argv[]) } break; + case 's': + if (virStrToLong_i(optarg, NULL, 10, &handshakefd) < 0) { + fprintf(stderr, "malformed --handshakefd argument '%s'", + optarg); + goto cleanup; + } + break; + case 'h': case '?': fprintf(stderr, "\n"); @@ -845,6 +864,7 @@ int main(int argc, char *argv[]) fprintf(stderr, " -n NAME, --name NAME\n"); fprintf(stderr, " -c FD, --console FD\n"); fprintf(stderr, " -v VETH, --veth VETH\n"); + fprintf(stderr, " -s FD, --handshakefd FD\n"); fprintf(stderr, " -h, --help\n"); fprintf(stderr, "\n"); goto cleanup; @@ -862,6 +882,12 @@ int main(int argc, char *argv[]) goto cleanup; } + if (handshakefd < 0) { + fprintf(stderr, "%s: missing --handshake argument for container PTY\n", + argv[0]); + goto cleanup; + } + if (getuid() != 0) { fprintf(stderr, "%s: must be run as the 'root' user\n", argv[0]); goto cleanup; @@ -932,7 +958,8 @@ int main(int argc, char *argv[]) goto cleanup; } - rc = lxcControllerRun(def, nveths, veths, monitor, client, appPty); + rc = lxcControllerRun(def, nveths, veths, monitor, client, appPty, + handshakefd); cleanup: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 99f94ff..755de70 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1287,7 +1287,8 @@ lxcBuildControllerCmd(lxc_driver_t *driver, int nveths, char **veths, int appPty, - int logfile) + int logfile, + int handshakefd) { int i; char *filterstr; @@ -1332,6 +1333,8 @@ lxcBuildControllerCmd(lxc_driver_t *driver, virCommandAddArgList(cmd, "--name", vm->def->name, "--console", NULL); virCommandAddArgFormat(cmd, "%d", appPty); + virCommandAddArg(cmd, "--handshake"); + virCommandAddArgFormat(cmd, "%d", handshakefd); virCommandAddArg(cmd, "--background"); for (i = 0 ; i < nveths ; i++) { @@ -1355,6 +1358,7 @@ lxcBuildControllerCmd(lxc_driver_t *driver, } virCommandPreserveFD(cmd, appPty); + virCommandPreserveFD(cmd, handshakefd); virCommandSetOutputFD(cmd, &logfile); virCommandSetErrorFD(cmd, &logfile); @@ -1364,6 +1368,78 @@ cleanup: return NULL; } +static int +lxcReadLogOutput(virDomainObjPtr vm, + char *logfile, + off_t pos, + char *buf, + size_t buflen) +{ + int fd; + off_t off; + int whence; + int got = 0, ret = -1; + int retries = 10; + + if ((fd = open(logfile, O_RDONLY)) < 0) { + virReportSystemError(errno, _("failed to open logfile %s"), + logfile); + goto cleanup; + } + + if (pos < 0) { + off = 0; + whence = SEEK_END; + } else { + off = pos; + whence = SEEK_SET; + } + + if (lseek(fd, off, whence) < 0) { + if (whence == SEEK_END) + virReportSystemError(errno, + _("unable to seek to end of log for %s"), + logfile); + else + virReportSystemError(errno, + _("unable to seek to %lld from start for %s"), + (long long)off, logfile); + goto cleanup; + } + + while (retries) { + ssize_t bytes; + int isdead = 0; + + if (kill(vm->pid, 0) == -1 && errno == ESRCH) + isdead = 1; + + /* Any failures should be detected before we read the log, so we + * always have something useful to report on failure. */ + bytes = saferead(fd, buf+got, buflen-got-1); + if (bytes < 0) { + virReportSystemError(errno, "%s", + _("Failure while reading guest log output")); + goto cleanup; + } + + got += bytes; + buf[got] = '\0'; + + if ((got == buflen-1) || isdead) { + break; + } + + usleep(100*1000); + retries--; + } + + + ret = got; +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; +} /** * lxcVmStart: @@ -1389,6 +1465,7 @@ static int lxcVmStart(virConnectPtr conn, int logfd = -1; unsigned int nveths = 0; char **veths = NULL; + int handshakefds[2] = { -1, -1 }; off_t pos = -1; char ebuf[1024]; char *timestamp; @@ -1462,10 +1539,16 @@ static int lxcVmStart(virConnectPtr conn, goto cleanup; } + if (pipe(handshakefds) < 0) { + virReportSystemError(errno, "%s", + _("Unable to create pipe")); + goto cleanup; + } + if (!(cmd = lxcBuildControllerCmd(driver, vm, nveths, veths, - parentTty, logfd))) + parentTty, logfd, handshakefds[1]))) goto cleanup; /* Log timestamp */ @@ -1489,6 +1572,12 @@ static int lxcVmStart(virConnectPtr conn, if (virCommandRun(cmd, NULL) < 0) goto cleanup; + if (VIR_CLOSE(handshakefds[1]) < 0) { + virReportSystemError(errno, "%s", _("could not close handshake fd")); + goto cleanup; + } + handshakefds[1] = -1; + /* Connect to the controller as a client *first* because * this will block until the child has written their * pid file out to disk */ @@ -1506,6 +1595,18 @@ static int lxcVmStart(virConnectPtr conn, vm->def->id = vm->pid; virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason); + if (lxcContainerWaitForContinue(handshakefds[0]) < 0) { + char out[1024]; + + if (!(lxcReadLogOutput(vm, logfile, pos, out, 1024) < 0)) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("guest failed to start: %s"), out); + } + + lxcVmTerminate(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); + goto cleanup; + } + if ((priv->monitorWatch = virEventAddHandle( priv->monitor, VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, @@ -1545,6 +1646,8 @@ cleanup: if (rc != 0) VIR_FORCE_CLOSE(priv->monitor); VIR_FORCE_CLOSE(parentTty); + VIR_FORCE_CLOSE(handshakefds[0]); + VIR_FORCE_CLOSE(handshakefds[1]); VIR_FREE(logfile); return rc; } -- 1.7.4.4

On 06/02/2011 04:19 PM, Eric Blake wrote: Thanks, pushed series with this cleanup and the spelling fix in patch 3. - Cole

On 06/02/2011 01:40 PM, Cole Robinson wrote: Interesting - pipe() in the last patch, and socketpair() in this patch. At any rate, the underlying use of the fd pair is the same. ACK, post-release. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 06/02/2011 01:40 PM, Cole Robinson wrote: ACK. This one's simple enough and qualifies as a bug-fix that you could rebase it to push now prior to 0.9.2; or you can just wait until the release and push with the rest of the series. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org