[PATCH 5/7] qemu: agent: Make setting of vcpus more robust

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]