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

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 28e525294e20191f00e27b7894ea0536f53b15f6..09a1b8e38ae8f0baa042db27348ed09ad922e5c7 100644 (file)
--- 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 0adef813a0788dfca4ac5ccb179938ec5e14e7d4..910865b7d8fd6974e9e2beb4477ca80c462d8bf2 100644 (file)
--- 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 bdd0dcb3badfa43d274a88d25e8974ca3467ab29..75adf661cc6e52093403ae18cfa94a58a03cc1f3 100644 (file)
--- 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 106f2d3770f4a354fb439a31287206fbb8163054..3b42e7ab432188985fe69e5c83475f91318ca457 100644 (file)
--- 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;
     }