[libvirt] [PATCH] qemu: cleanup error checking on agent replies

[Martin Kletzander]

On Tue, Apr 01, 2014 at 08:26:57AM -0600, Eric Blake wrote:
>On 04/01/2014 07:22 AM, Martin Kletzander wrote:
>> On all the places where qemuAgentComand() was called, we did a check
>> for errors in the reply.  Unfortunately, some of the places called
>> qemuAgentCheckError() without checking for non-null reply which might
>> have resulted in a crash.
>>
>> So this patch makes the error-checking part of qemuAgentCommand()
>> itself, which:
>>
>>  a) makes it look better,
>>
>>  b) makes the check mandatory and, most importantly,
>>
>>  c) checks for the errors if and only if it is appropriate.
>>
>> This actually fixes a potential crashers when qemuAgentComand()
>> returned 0, but reply was NULL.  Having said that, it *should* fix the
>> following bug:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1058149
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>  src/qemu/qemu_agent.c | 27 +++++++--------------------
>>  1 file changed, 7 insertions(+), 20 deletions(-)
>>
>
>>
>> +static int qemuAgentCheckError(virJSONValuePtr cmd, virJSONValuePtr reply);
>> +
>
>Is it worth hoisting this function into topological order, so we don't
>need a forward declaration?   But that's better as a followup patch
>(no-op code motion should be separate from refactoring).
>
>ACK
>

Thank you, I've pushed it.  Would the following suffice as the mentioned
follow-up?  Or do you want me to send a new standalone patch for that?

Martin

diff --git i/src/qemu/qemu_agent.c w/src/qemu/qemu_agent.c
index 4c83d7d..92573bd 100644
--- i/src/qemu/qemu_agent.c
+++ w/src/qemu/qemu_agent.c
@@ -117,8 +117,6 @@ struct _qemuAgent {
     qemuAgentEvent await_event;
 };

-static int qemuAgentCheckError(virJSONValuePtr cmd, virJSONValuePtr reply);
-
 static virClassPtr qemuAgentClass;
 static void qemuAgentDispose(void *obj);

@@ -967,61 +965,6 @@ qemuAgentGuestSync(qemuAgentPtr mon)
     return ret;
 }

-static int
-qemuAgentCommand(qemuAgentPtr mon,
-                 virJSONValuePtr cmd,
-                 virJSONValuePtr *reply,
-                 int seconds)
-{
-    int ret = -1;
-    qemuAgentMessage msg;
-    char *cmdstr = NULL;
-    int await_event = mon->await_event;
-
-    *reply = NULL;
-
-    if (qemuAgentGuestSync(mon) < 0)
-        return -1;
-
-    memset(&msg, 0, sizeof(msg));
-
-    if (!(cmdstr = virJSONValueToString(cmd, false)))
-        goto cleanup;
-    if (virAsprintf(&msg.txBuffer, "%s" LINE_ENDING, cmdstr) < 0)
-        goto cleanup;
-    msg.txLength = strlen(msg.txBuffer);
-
-    VIR_DEBUG("Send command '%s' for write, seconds = %d", cmdstr, seconds);
-
-    ret = qemuAgentSend(mon, &msg, seconds);
-
-    VIR_DEBUG("Receive command reply ret=%d rxObject=%p",
-              ret, msg.rxObject);
-
-    if (ret == 0) {
-        /* If we haven't obtained any reply but we wait for an
-         * event, then don't report this as error */
-        if (!msg.rxObject) {
-            if (await_event) {
-                VIR_DEBUG("Woken up by event %d", await_event);
-            } else {
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("Missing monitor reply object"));
-                ret = -1;
-            }
-        } else {
-            *reply = msg.rxObject;
-            ret = qemuAgentCheckError(cmd, *reply);
-        }
-    }
-
- cleanup:
-    VIR_FREE(cmdstr);
-    VIR_FREE(msg.txBuffer);
-
-    return ret;
-}
-
 static const char *
 qemuAgentStringifyErrorClass(const char *klass)
 {
@@ -1133,6 +1076,61 @@ qemuAgentCheckError(virJSONValuePtr cmd,
     return 0;
 }

+static int
+qemuAgentCommand(qemuAgentPtr mon,
+                 virJSONValuePtr cmd,
+                 virJSONValuePtr *reply,
+                 int seconds)
+{
+    int ret = -1;
+    qemuAgentMessage msg;
+    char *cmdstr = NULL;
+    int await_event = mon->await_event;
+
+    *reply = NULL;
+
+    if (qemuAgentGuestSync(mon) < 0)
+        return -1;
+
+    memset(&msg, 0, sizeof(msg));
+
+    if (!(cmdstr = virJSONValueToString(cmd, false)))
+        goto cleanup;
+    if (virAsprintf(&msg.txBuffer, "%s" LINE_ENDING, cmdstr) < 0)
+        goto cleanup;
+    msg.txLength = strlen(msg.txBuffer);
+
+    VIR_DEBUG("Send command '%s' for write, seconds = %d", cmdstr, seconds);
+
+    ret = qemuAgentSend(mon, &msg, seconds);
+
+    VIR_DEBUG("Receive command reply ret=%d rxObject=%p",
+              ret, msg.rxObject);
+
+    if (ret == 0) {
+        /* If we haven't obtained any reply but we wait for an
+         * event, then don't report this as error */
+        if (!msg.rxObject) {
+            if (await_event) {
+                VIR_DEBUG("Woken up by event %d", await_event);
+            } else {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("Missing monitor reply object"));
+                ret = -1;
+            }
+        } else {
+            *reply = msg.rxObject;
+            ret = qemuAgentCheckError(cmd, *reply);
+        }
+    }
+
+ cleanup:
+    VIR_FREE(cmdstr);
+    VIR_FREE(msg.txBuffer);
+
+    return ret;
+}
+
 static virJSONValuePtr ATTRIBUTE_SENTINEL
 qemuAgentMakeCommand(const char *cmdname,
                      ...)
--
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140402/39e2379c/attachment-0001.sig>