1:12 a.m.
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(a)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,
...)
--