qemu: agent: Make setting of vcpus more robust

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 malicious guest agent
- fix testsuite to the new semantics

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;
@@ -1548,6 +1547,12 @@ qemuAgentSetVCPUs(qemuAgentPtr mon,
     for (i = 0; i < ninfo; i++) {
         qemuAgentCPUInfoPtr in = &info[i];
 
+        /* don't set state for cpus that were not touched */
+        if (!in->modified)
+            continue;
+
+        (*nmodified)++;
+
         /* create single cpu object */
         if (!(cpu = virJSONValueNewObject()))
             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 bogus since we wouldn't
+     * call the guest agent so that 0 cpus would be set successfully. Reporting
+     * more successfully 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 {

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);

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);
 

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;