[libvirt] [PATCH 5/7] qemu: agent: Make setting of vcpus more robust
Peter Krempa
pkrempa at redhat.com
Mon Jun 20 14:34:19 UTC 2016
Documentation for the "guest-set-vcpus" command describes a proper
algorithm how to set vcpus. This patch makes the following changes:
- state of cpus that has not changed is not updated
- if the command was partially successful the command is re-tried with
the rest of the arguments to get a proper error message
- code is more robust against mailicious guest agent
- fix testsuite to the new semantics
---
src/qemu/qemu_agent.c | 83 ++++++++++++++++++++++++++++++++++++++++++--------
src/qemu/qemu_agent.h | 2 ++
src/qemu/qemu_driver.c | 13 --------
tests/qemuagenttest.c | 44 ++++++++++++--------------
4 files changed, 92 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index cbc0995..5bd767a 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1523,16 +1523,13 @@ qemuAgentGetVCPUs(qemuAgentPtr mon,
return ret;
}
-/**
- * Set the VCPU state using guest agent.
- *
- * Returns -1 on error, ninfo in case everything was successful and less than
- * ninfo on a partial failure.
- */
-int
-qemuAgentSetVCPUs(qemuAgentPtr mon,
- qemuAgentCPUInfoPtr info,
- size_t ninfo)
+
+/* returns the value provided by the guest agent or -1 on internal error */
+static int
+qemuAgentSetVCPUsCommand(qemuAgentPtr mon,
+ qemuAgentCPUInfoPtr info,
+ size_t ninfo,
+ int *nmodified)
{
int ret = -1;
virJSONValuePtr cmd = NULL;
@@ -1541,6 +1538,8 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
virJSONValuePtr cpu = NULL;
size_t i;
+ *nmodified = 0;
+
/* create the key data array */
if (!(cpus = virJSONValueNewArray()))
goto cleanup;
@@ -1552,6 +1551,12 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
if (!(cpu = virJSONValueNewObject()))
goto cleanup;
+ /* don't set state for cpus that were not touched */
+ if (!in->modified)
+ continue;
+
+ (*nmodified)++;
+
if (virJSONValueObjectAppendNumberInt(cpu, "logical-id", in->id) < 0)
goto cleanup;
@@ -1564,6 +1569,11 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
cpu = NULL;
}
+ if (*nmodified == 0) {
+ ret = 0;
+ goto cleanup;
+ }
+
if (!(cmd = qemuAgentMakeCommand("guest-set-vcpus",
"a:vcpus", cpus,
NULL)))
@@ -1575,9 +1585,17 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
goto cleanup;
- if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
+ if (qemuAgentCheckError(cmd, reply) < 0)
+ goto cleanup;
+
+ /* All negative values are invalid. Return of 0 is bougs since we wouldn't
+ * call the guest agent so that 0 cpus would be set successfully. Reporting
+ * more successfuly set vcpus that we've asked for is invalid */
+ if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0 ||
+ ret <= 0 || ret > *nmodified) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("malformed return value"));
+ _("guest agent returned malformed or invalid return value"));
+ ret = -1;
}
cleanup:
@@ -1589,6 +1607,45 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
}
+/**
+ * Set the VCPU state using guest agent.
+ *
+ * Attempts to set the guest agent state for all cpus or until a proper error is
+ * reported by the guest agent. This may require multiple calls.
+ *
+ * Returns -1 on error, 0 on success.
+ */
+int
+qemuAgentSetVCPUs(qemuAgentPtr mon,
+ qemuAgentCPUInfoPtr info,
+ size_t ninfo)
+{
+ int rv;
+ int nmodified;
+ size_t i;
+
+ do {
+ if ((rv = qemuAgentSetVCPUsCommand(mon, info, ninfo, &nmodified)) < 0)
+ return -1;
+
+ /* all vcpus were set successfully */
+ if (rv == nmodified)
+ return 0;
+
+ /* un-mark vcpus that were already set */
+ for (i = 0; i < ninfo && rv > 0; i++) {
+ if (!info[i].modified)
+ continue;
+
+ info[i].modified = false;
+ rv--;
+ }
+ } while (1);
+
+ return 0;
+}
+
+
/* modify the cpu info structure to set the correct amount of cpus */
int
qemuAgentUpdateCPUInfo(unsigned int nvcpus,
@@ -1647,12 +1704,14 @@ qemuAgentUpdateCPUInfo(unsigned int nvcpus,
/* unplug */
if (cpuinfo[i].offlinable && cpuinfo[i].online) {
cpuinfo[i].online = false;
+ cpuinfo[i].modified = true;
nonline--;
}
} else if (nvcpus > nonline) {
/* plug */
if (!cpuinfo[i].online) {
cpuinfo[i].online = true;
+ cpuinfo[i].modified = true;
nonline++;
}
} else {
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index c092504..6dd9c70 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -95,6 +95,8 @@ struct _qemuAgentCPUInfo {
unsigned int id; /* logical cpu ID */
bool online; /* true if the CPU is activated */
bool offlinable; /* true if the CPU can be offlined */
+
+ bool modified; /* set to true if the vcpu state needs to be changed */
};
int qemuAgentGetVCPUs(qemuAgentPtr mon, qemuAgentCPUInfoPtr *info);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d065e45..098840f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4804,19 +4804,6 @@ qemuDomainSetVcpusAgent(virDomainObjPtr vm,
ret = qemuAgentSetVCPUs(qemuDomainGetAgent(vm), cpuinfo, ncpuinfo);
qemuDomainObjExitAgent(vm);
- if (ret < 0)
- goto cleanup;
-
- if (ret < ncpuinfo) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to set state of cpu %d via guest agent"),
- cpuinfo[ret-1].id);
- ret = -1;
- goto cleanup;
- }
-
- ret = 0;
-
cleanup:
VIR_FREE(cpuinfo);
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 60dafd9..4aa2c49 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -517,17 +517,15 @@ static const char testQemuAgentCPUResponse[] =
"}";
static const char testQemuAgentCPUArguments1[] =
- "[{\"logical-id\":0,\"online\":true},"
- "{\"logical-id\":1,\"online\":false},"
- "{\"logical-id\":2,\"online\":true},"
- "{\"logical-id\":3,\"online\":false}]";
+ "[{\"logical-id\":1,\"online\":false}]";
static const char testQemuAgentCPUArguments2[] =
- "[{\"logical-id\":0,\"online\":true},"
- "{\"logical-id\":1,\"online\":true},"
- "{\"logical-id\":2,\"online\":true},"
+ "[{\"logical-id\":1,\"online\":true},"
"{\"logical-id\":3,\"online\":true}]";
+static const char testQemuAgentCPUArguments3[] =
+ "[{\"logical-id\":3,\"online\":true}]";
+
static int
testQemuAgentCPU(const void *data)
{
@@ -566,43 +564,39 @@ testQemuAgentCPU(const void *data)
goto cleanup;
if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus",
- "{ \"return\" : 4 }",
+ "{ \"return\" : 1 }",
"vcpus", testQemuAgentCPUArguments1,
NULL) < 0)
goto cleanup;
- if ((nvcpus = qemuAgentSetVCPUs(qemuMonitorTestGetAgent(test),
- cpuinfo, nvcpus)) < 0)
- goto cleanup;
-
- if (nvcpus != 4) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "Expected '4' cpus updated , got '%d'", nvcpus);
+ if (qemuAgentSetVCPUs(qemuMonitorTestGetAgent(test), cpuinfo, nvcpus) < 0)
goto cleanup;
- }
- /* try to hotplug two */
+ /* try to hotplug two, second one will fail*/
if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
goto cleanup;
if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus",
- "{ \"return\" : 4 }",
+ "{ \"return\" : 1 }",
"vcpus", testQemuAgentCPUArguments2,
NULL) < 0)
goto cleanup;
- if (qemuAgentUpdateCPUInfo(4, cpuinfo, nvcpus) < 0)
+ if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
goto cleanup;
- if ((nvcpus = qemuAgentSetVCPUs(qemuMonitorTestGetAgent(test),
- cpuinfo, nvcpus)) < 0)
+ if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus",
+ "{ \"error\" : \"random error\" }",
+ "vcpus", testQemuAgentCPUArguments3,
+ NULL) < 0)
goto cleanup;
- if (nvcpus != 4) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "Expected '4' cpus updated , got '%d'", nvcpus);
+ if (qemuAgentUpdateCPUInfo(4, cpuinfo, nvcpus) < 0)
+ goto cleanup;
+
+ /* this should fail */
+ if (qemuAgentSetVCPUs(qemuMonitorTestGetAgent(test), cpuinfo, nvcpus) != -1)
goto cleanup;
- }
ret = 0;
--
2.8.3
More information about the libvir-list
mailing list