[libvirt] [PATCH] qemu: Report cmdline output if VM dies early

qemuReadLogOutput early VM death detection is racy and won't always work. Startup then errors when connecting to the VM monitor. This won't report the emulator cmdline output which is typically the most useful diagnostic. Check if the VM has died at the very end of the monitor connection step, and if so, report the cmdline output. See also: https://bugzilla.redhat.com/show_bug.cgi?id=581381 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_driver.c | 59 ++++++++++++++++++++++++++++++++++------------- 1 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42a653a..c446028 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2029,39 +2029,47 @@ static void qemudFreePtyPath(void *payload, const char *name ATTRIBUTE_UNUSED) VIR_FREE(payload); } +static void +qemuReadLogFD(int logfd, char *buf, int maxlen, int off) +{ + int ret; + char *tmpbuf = buf+off; + + ret = saferead(logfd, tmpbuf, maxlen-off-1); + if (ret < 0) { + ret = 0; + } + + *(tmpbuf+ret) = '\0'; +} + static int qemudWaitForMonitor(struct qemud_driver* driver, virDomainObjPtr vm, off_t pos) { - char buf[4096]; /* Plenty of space to get startup greeting */ + char buf[4096] = "\0"; /* Plenty of space to get startup greeting */ int logfd; int ret = -1; + virHashTablePtr paths = NULL; - if ((logfd = qemudLogReadFD(driver->logDir, vm->def->name, pos)) - < 0) + if ((logfd = qemudLogReadFD(driver->logDir, vm->def->name, pos)) < 0) return -1; - ret = qemudReadLogOutput(vm, logfd, buf, sizeof(buf), - qemudFindCharDevicePTYs, - "console", 30); - if (close(logfd) < 0) { - char ebuf[4096]; - VIR_WARN(_("Unable to close logfile: %s"), - virStrerror(errno, ebuf, sizeof ebuf)); - } - - if (ret < 0) - return -1; + if (qemudReadLogOutput(vm, logfd, buf, sizeof(buf), + qemudFindCharDevicePTYs, + "console", 30) < 0) + goto closelog; VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name); - if (qemuConnectMonitor(driver, vm) < 0) - return -1; + if (qemuConnectMonitor(driver, vm) < 0) { + goto cleanup; + } /* Try to get the pty path mappings again via the monitor. This is much more * reliable if it's available. * Note that the monitor itself can be on a pty, so we still need to try the * log output method. */ - virHashTablePtr paths = virHashCreate(0); + paths = virHashCreate(0); if (paths == NULL) { virReportOOMError(); goto cleanup; @@ -2082,6 +2090,23 @@ cleanup: virHashFree(paths, qemudFreePtyPath); } + if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + /* VM is dead, any other error raised in the interim is probably + * not as important as the qemu cmdline output */ + qemuReadLogFD(logfd, buf, sizeof(buf), strlen(buf)); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + ret = -1; + } + +closelog: + if (close(logfd) < 0) { + char ebuf[4096]; + VIR_WARN(_("Unable to close logfile: %s"), + virStrerror(errno, ebuf, sizeof ebuf)); + } + return ret; } -- 1.7.0.1

On 04/30/2010 09:44 AM, Cole Robinson wrote:
qemuReadLogOutput early VM death detection is racy and won't always work. Startup then errors when connecting to the VM monitor. This won't report the emulator cmdline output which is typically the most useful diagnostic.
Check if the VM has died at the very end of the monitor connection step, and if so, report the cmdline output.
+static void +qemuReadLogFD(int logfd, char *buf, int maxlen, int off) +{ + int ret; + char *tmpbuf = buf+off;
Isn't the style to separate operators by space? 'buf + off'
+ + ret = saferead(logfd, tmpbuf, maxlen-off-1);
Likewise, 'maxlen - off - 1'.
+ if (ret < 0) { + ret = 0; + } + + *(tmpbuf+ret) = '\0';
Stylistically, I like 'tmpbuf[ret]' better than '*(tmpbuf+ret)'.
+} + static int qemudWaitForMonitor(struct qemud_driver* driver, virDomainObjPtr vm, off_t pos) { - char buf[4096]; /* Plenty of space to get startup greeting */ + char buf[4096] = "\0"; /* Plenty of space to get startup greeting */
"" is sufficient; you don't have to use "\0" to zero-initialize a statically sized char[]. But overall, the patch looks sane; I didn't see any logic errors. ACK with the style nits addressed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 05/04/2010 04:58 PM, Eric Blake wrote:
On 04/30/2010 09:44 AM, Cole Robinson wrote:
qemuReadLogOutput early VM death detection is racy and won't always work. Startup then errors when connecting to the VM monitor. This won't report the emulator cmdline output which is typically the most useful diagnostic.
Check if the VM has died at the very end of the monitor connection step, and if so, report the cmdline output.
+static void +qemuReadLogFD(int logfd, char *buf, int maxlen, int off) +{ + int ret; + char *tmpbuf = buf+off;
Isn't the style to separate operators by space? 'buf + off'
+ + ret = saferead(logfd, tmpbuf, maxlen-off-1);
Likewise, 'maxlen - off - 1'.
+ if (ret < 0) { + ret = 0; + } + + *(tmpbuf+ret) = '\0';
Stylistically, I like 'tmpbuf[ret]' better than '*(tmpbuf+ret)'.
Doh, I was overthinking here :)
+} + static int qemudWaitForMonitor(struct qemud_driver* driver, virDomainObjPtr vm, off_t pos) { - char buf[4096]; /* Plenty of space to get startup greeting */ + char buf[4096] = "\0"; /* Plenty of space to get startup greeting */
"" is sufficient; you don't have to use "\0" to zero-initialize a statically sized char[].
But overall, the patch looks sane; I didn't see any logic errors. ACK with the style nits addressed.
Thanks for the review, I made these style adjustments and pushed. - Cole
participants (2)
-
Cole Robinson
-
Eric Blake