[libvirt] [PATCH 3/3] Issue full error messages when processing QEMU monitor I/O

Currently the QEMU monitor I/O handler code uses errno values to report errors. This results in a sub-optimal error messages on certain conditions, in particular when parsing JSON strings malformed data simply results in 'EINVAL'. This changes the code to use the standard libvirt error reporting APIs. The virError is stored against the qemuMonitorPtr struct, and when a monitor API is run, any existing stored error is copied into that thread's error local * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h, src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_text.c: Use virError APIs for all monitor I/O handling code --- src/qemu/qemu_monitor.c | 161 ++++++++++++++++++++++++++++-------------- src/qemu/qemu_monitor.h | 11 ++- src/qemu/qemu_monitor_json.c | 85 +++++++++++----------- src/qemu/qemu_monitor_text.c | 51 +++++++------- 4 files changed, 183 insertions(+), 125 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 28e5252..09a1b8e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -69,7 +69,9 @@ struct _qemuMonitor { /* If anything went wrong, this will be fed back * the next monitor msg */ - int lastErrno; + virError lastError; + + int nextSerial; unsigned json: 1; unsigned json_hmp: 1; @@ -312,6 +314,10 @@ qemuMonitorOpenPty(const char *monitor) } +/* This method processes data that has been received + * from the monitor. Looking for async events and + * replies/errors. + */ static int qemuMonitorIOProcess(qemuMonitorPtr mon) { @@ -344,10 +350,8 @@ qemuMonitorIOProcess(qemuMonitorPtr mon) mon->buffer, mon->bufferOffset, msg); - if (len < 0) { - mon->lastErrno = errno; + if (len < 0) return -1; - } if (len < mon->bufferOffset) { memmove(mon->buffer, mon->buffer + len, mon->bufferOffset - len); @@ -378,11 +382,6 @@ qemuMonitorIOWriteWithFD(qemuMonitorPtr mon, char control[CMSG_SPACE(sizeof(int))]; struct cmsghdr *cmsg; - if (!mon->hasSendFD) { - errno = EINVAL; - return -1; - } - memset(&msg, 0, sizeof(msg)); iov[0].iov_base = (void *)data; @@ -423,6 +422,12 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) if (!mon->msg || mon->msg->txOffset == mon->msg->txLength) return 0; + if (mon->msg->txFD != -1 && !mon->hasSendFD) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Monitor does not support sending of file descriptors")); + return -1; + } + if (mon->msg->txFD == -1) done = write(mon->fd, mon->msg->txBuffer + mon->msg->txOffset, @@ -437,7 +442,8 @@ qemuMonitorIOWrite(qemuMonitorPtr mon) if (errno == EAGAIN) return 0; - mon->lastErrno = errno; + virReportSystemError(errno, "%s", + _("Unable to write to monitor")); return -1; } mon->msg->txOffset += done; @@ -459,7 +465,7 @@ qemuMonitorIORead(qemuMonitorPtr mon) if (avail < 1024) { if (VIR_REALLOC_N(mon->buffer, mon->bufferLength + 1024) < 0) { - errno = ENOMEM; + virReportOOMError(); return -1; } mon->bufferLength += 1024; @@ -476,7 +482,8 @@ qemuMonitorIORead(qemuMonitorPtr mon) if (got < 0) { if (errno == EAGAIN) break; - mon->lastErrno = errno; + virReportSystemError(errno, "%s", + _("Unable to read from monitor")); ret = -1; break; } @@ -503,7 +510,7 @@ static void qemuMonitorUpdateWatch(qemuMonitorPtr mon) VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR; - if (!mon->lastErrno) { + if (mon->lastError.code == VIR_ERR_OK) { events |= VIR_EVENT_HANDLE_READABLE; if (mon->msg && mon->msg->txOffset < mon->msg->txLength) @@ -528,61 +535,84 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) { #endif if (mon->fd != fd || mon->watch != watch) { - VIR_ERROR(_("event from unexpected fd %d!=%d / watch %d!=%d"), mon->fd, fd, mon->watch, watch); + if (events & VIR_EVENT_HANDLE_HANGUP) + eof = true; + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("event from unexpected fd %d!=%d / watch %d!=%d"), + mon->fd, fd, mon->watch, watch); + error = true; + } else if (mon->lastError.code != VIR_ERR_OK) { + if (events & VIR_EVENT_HANDLE_HANGUP) + eof = true; error = true; } else { - if (!mon->lastErrno && - events & VIR_EVENT_HANDLE_WRITABLE) { - int done = qemuMonitorIOWrite(mon); - if (done < 0) - error = 1; + if (events & VIR_EVENT_HANDLE_WRITABLE) { + if (qemuMonitorIOWrite(mon) < 0) + error = true; events &= ~VIR_EVENT_HANDLE_WRITABLE; } - if (!mon->lastErrno && + + if (!error && events & VIR_EVENT_HANDLE_READABLE) { int got = qemuMonitorIORead(mon); - if (got < 0) + events &= ~VIR_EVENT_HANDLE_READABLE; + if (got < 0) { error = true; - /* Ignore hangup/error events if we read some data, to - * give time for that data to be consumed */ - if (got > 0) { + } else if (got == 0) { + eof = true; + } else { + /* Ignore hangup/error events if we read some data, to + * give time for that data to be consumed */ events = 0; if (qemuMonitorIOProcess(mon) < 0) error = true; - } else - events &= ~VIR_EVENT_HANDLE_READABLE; + } } - /* If IO process resulted in an error & we have a message, - * then wakeup that waiter */ - if (mon->lastErrno && mon->msg && !mon->msg->finished) { - mon->msg->lastErrno = mon->lastErrno; - mon->msg->finished = 1; - virCondSignal(&mon->notify); + if (!error && + events & VIR_EVENT_HANDLE_HANGUP) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("End of file from monitor")); + eof = 1; + events &= ~VIR_EVENT_HANDLE_HANGUP; } - qemuMonitorUpdateWatch(mon); - - if (events & (VIR_EVENT_HANDLE_HANGUP | - VIR_EVENT_HANDLE_ERROR)) { - /* If IO process resulted in EOF & we have a message, - * then wakeup that waiter */ - if (mon->msg && !mon->msg->finished) { - mon->msg->finished = 1; - mon->msg->lastErrno = EIO; - virCondSignal(&mon->notify); - } - eof = 1; - } else if (events) { - VIR_ERROR(_("unhandled fd event %d for monitor fd %d"), - events, mon->fd); + if (!error && !eof && + events & VIR_EVENT_HANDLE_ERROR) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Error while waiting for monitor")); + error = 1; + } + if (!error && events) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unhandled event %d for monitor fd %d"), + events, mon->fd); error = 1; } } - if (eof || error) - mon->lastErrno = EIO; + if (error || eof) { + if (mon->lastError.code != VIR_ERR_OK) { + /* Already have an error, so clear any new error */ + virResetLastError(); + } else { + virErrorPtr err = virGetLastError(); + if (!err) + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Error while processing monitor IO")); + virCopyLastError(&mon->lastError); + virResetLastError(); + } + + VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message)); + /* If IO process resulted in an error & we have a message, + * then wakeup that waiter */ + if (mon->msg && !mon->msg->finished) { + mon->msg->finished = 1; + virCondSignal(&mon->notify); + } + } qemuMonitorUpdateWatch(mon); @@ -738,14 +768,28 @@ void qemuMonitorClose(qemuMonitorPtr mon) } +char *qemuMonitorNextCommandID(qemuMonitorPtr mon) +{ + char *id; + + if (virAsprintf(&id, "libvirt-%d", ++mon->nextSerial) < 0) { + virReportOOMError(); + return NULL; + } + return id; +} + + int qemuMonitorSend(qemuMonitorPtr mon, qemuMonitorMessagePtr msg) { int ret = -1; /* Check whether qemu quited unexpectedly */ - if (mon->lastErrno) { - msg->lastErrno = mon->lastErrno; + if (mon->lastError.code != VIR_ERR_OK) { + VIR_DEBUG("Attempt to send command while error is set %s", + NULLSTR(mon->lastError.message)); + virSetError(&mon->lastError); return -1; } @@ -753,12 +797,21 @@ int qemuMonitorSend(qemuMonitorPtr mon, qemuMonitorUpdateWatch(mon); while (!mon->msg->finished) { - if (virCondWait(&mon->notify, &mon->lock) < 0) + if (virCondWait(&mon->notify, &mon->lock) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to wait on monitor condition")); goto cleanup; + } } - if (mon->lastErrno == 0) - ret = 0; + if (mon->lastError.code != VIR_ERR_OK) { + VIR_DEBUG("Send command resulted in error %s", + NULLSTR(mon->lastError.message)); + virSetError(&mon->lastError); + goto cleanup; + } + + ret = 0; cleanup: mon->msg = NULL; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 0adef81..910865b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -49,12 +49,16 @@ struct _qemuMonitorMessage { int txOffset; int txLength; + /* Used by the text monitor reply / error */ char *rxBuffer; int rxLength; + /* Used by the JSON monitor to hold reply / error */ + void *rxObject; - int finished; - - int lastErrno; + /* True if rxBuffer / rxObject are ready, or a + * fatal error occurred on the monitor channel + */ + bool finished; qemuMonitorPasswordHandler passwordHandler; void *passwordOpaque; @@ -137,6 +141,7 @@ int qemuMonitorRef(qemuMonitorPtr mon); int qemuMonitorUnref(qemuMonitorPtr mon) ATTRIBUTE_RETURN_CHECK; /* These APIs are for use by the internal Text/JSON monitor impl code only */ +char *qemuMonitorNextCommandID(qemuMonitorPtr mon); int qemuMonitorSend(qemuMonitorPtr mon, qemuMonitorMessagePtr msg); int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2d8a390..87cd45d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -111,43 +111,38 @@ qemuMonitorJSONIOProcessLine(qemuMonitorPtr mon, VIR_DEBUG("Line [%s]", line); - if (!(obj = virJSONValueFromString(line))) { - VIR_DEBUG("Parsing JSON string failed"); - errno = EINVAL; + if (!(obj = virJSONValueFromString(line))) goto cleanup; - } if (obj->type != VIR_JSON_TYPE_OBJECT) { - VIR_DEBUG("Parsed JSON string isn't an object"); - errno = EINVAL; + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Parsed JSON reply '%s' isn't an object"), line); + goto cleanup; } if (virJSONValueObjectHasKey(obj, "QMP") == 1) { - VIR_DEBUG("Got QMP capabilities data"); ret = 0; - goto cleanup; - } - - if (virJSONValueObjectHasKey(obj, "event") == 1) { + virJSONValueFree(obj); + } else if (virJSONValueObjectHasKey(obj, "event") == 1) { ret = qemuMonitorJSONIOProcessEvent(mon, obj); - goto cleanup; - } - - if (msg) { - if (!(msg->rxBuffer = strdup(line))) { - errno = ENOMEM; - goto cleanup; + } else if (virJSONValueObjectHasKey(obj, "error") == 1 || + virJSONValueObjectHasKey(obj, "return") == 1) { + if (msg) { + msg->rxObject = obj; + msg->finished = 1; + ret = 0; + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected JSON reply '%s'"), line); } - msg->rxLength = strlen(line); - msg->finished = 1; } else { - VIR_DEBUG("Ignoring unexpected JSON message [%s]", line); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown JSON reply '%s'"), line); } - ret = 0; - cleanup: - virJSONValueFree(obj); + if (ret < 0) + virJSONValueFree(obj); return ret; } @@ -166,7 +161,7 @@ int qemuMonitorJSONIOProcess(qemuMonitorPtr mon, int got = nl - (data + used); char *line = strndup(data + used, got); if (!line) { - errno = ENOMEM; + virReportOOMError(); return -1; } used += got + strlen(LINE_ENDING); @@ -195,11 +190,24 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon, int ret = -1; qemuMonitorMessage msg; char *cmdstr = NULL; + char *id = NULL; + virJSONValuePtr exe; *reply = NULL; memset(&msg, 0, sizeof msg); + exe = virJSONValueObjectGet(cmd, "execute"); + if (exe) { + if (!(id = qemuMonitorNextCommandID(mon))) + goto cleanup; + if (virJSONValueObjectAppendString(cmd, "id", id) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to append command 'id' string")); + goto cleanup; + } + } + if (!(cmdstr = virJSONValueToString(cmd))) { virReportOOMError(); goto cleanup; @@ -215,33 +223,24 @@ qemuMonitorJSONCommandWithFd(qemuMonitorPtr mon, ret = qemuMonitorSend(mon, &msg); - VIR_DEBUG("Receive command reply ret=%d errno=%d %d bytes '%s'", - ret, msg.lastErrno, msg.rxLength, msg.rxBuffer); + VIR_DEBUG("Receive command reply ret=%d rxObject=%p", + ret, msg.rxObject); - /* If we got ret==0, but not reply data something rather bad - * went wrong, so lets fake an EIO error */ - if (!msg.rxBuffer && ret == 0) { - msg.lastErrno = EIO; - ret = -1; - } - if (ret == 0) { - if (!((*reply) = virJSONValueFromString(msg.rxBuffer))) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse JSON doc '%s'"), msg.rxBuffer); - goto cleanup; + if (!msg.rxObject) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing monitor reply object")); + ret = -1; + } else { + *reply = msg.rxObject; } } - if (ret < 0) - virReportSystemError(msg.lastErrno, - _("cannot send monitor command '%s'"), cmdstr); - cleanup: + VIR_FREE(id); VIR_FREE(cmdstr); VIR_FREE(msg.txBuffer); - VIR_FREE(msg.rxBuffer); return ret; } diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 106f2d3..3b42e7a 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -153,7 +153,8 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, #endif if (msg->passwordHandler) { int i; - /* Try and handle the prompt */ + /* Try and handle the prompt. The handler is required + * to report a normal libvirt error */ if (msg->passwordHandler(mon, msg, start, passwd - start + strlen(PASSWORD_PROMPT), @@ -168,7 +169,8 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, /* Handled, so skip forward over password prompt */ start = passwd; } else { - errno = EACCES; + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Password request seen, but no handler available")); return -1; } } @@ -183,8 +185,10 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED, * BASIC_PROMPT we can reasonably reliably cope */ if (want) { if (VIR_REALLOC_N(msg->rxBuffer, - msg->rxLength + want + 1) < 0) + msg->rxLength + want + 1) < 0) { + virReportOOMError(); return -1; + } memcpy(msg->rxBuffer + msg->rxLength, start, want); msg->rxLength += want; msg->rxBuffer[msg->rxLength] = '\0'; @@ -234,30 +238,26 @@ qemuMonitorTextCommandWithHandler(qemuMonitorPtr mon, ret = qemuMonitorSend(mon, &msg); - VIR_DEBUG("Receive command reply ret=%d errno=%d %d bytes '%s'", - ret, msg.lastErrno, msg.rxLength, msg.rxBuffer); + VIR_DEBUG("Receive command reply ret=%d rxLength=%d rxBuffer='%s'", + ret, msg.rxLength, msg.rxBuffer); /* Just in case buffer had some passwords in */ memset(msg.txBuffer, 0, msg.txLength); VIR_FREE(msg.txBuffer); - /* To make life safer for callers, already ensure there's at least an empty string */ - if (msg.rxBuffer) { - *reply = msg.rxBuffer; - } else { - *reply = strdup(""); - if (!*reply) { - virReportOOMError(); - return -1; + if (ret >= 0) { + /* To make life safer for callers, already ensure there's at least an empty string */ + if (msg.rxBuffer) { + *reply = msg.rxBuffer; + } else { + *reply = strdup(""); + if (!*reply) { + virReportOOMError(); + return -1; + } } } - if (ret < 0) { - virReportSystemError(msg.lastErrno, - _("cannot send monitor command '%s'"), cmd); - VIR_FREE(*reply); - } - return ret; } @@ -297,15 +297,16 @@ qemuMonitorSendDiskPassphrase(qemuMonitorPtr mon, pathStart = strstr(data, DISK_ENCRYPTION_PREFIX); pathEnd = strstr(data, DISK_ENCRYPTION_POSTFIX); if (!pathStart || !pathEnd || pathStart >= pathEnd) { - errno = -EINVAL; + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to extract disk path from %s"), + data); return -1; } /* Extra the path */ pathStart += strlen(DISK_ENCRYPTION_PREFIX); - path = strndup(pathStart, pathEnd - pathStart); - if (!path) { - errno = ENOMEM; + if (!(path = strndup(pathStart, pathEnd - pathStart))) { + virReportOOMError(); return -1; } @@ -325,7 +326,7 @@ qemuMonitorSendDiskPassphrase(qemuMonitorPtr mon, msg->txLength + passphrase_len + 1 + 1) < 0) { memset(passphrase, 0, passphrase_len); VIR_FREE(passphrase); - errno = ENOMEM; + virReportOOMError(); return -1; } @@ -763,7 +764,7 @@ qemuMonitorSendVNCPassphrase(qemuMonitorPtr mon ATTRIBUTE_UNUSED, * to be sent back */ if (VIR_REALLOC_N(msg->txBuffer, msg->txLength + passphrase_len + 1 + 1) < 0) { - errno = ENOMEM; + virReportOOMError(); return -1; } -- 1.7.4.4