blockjob: fix block-stream bandwidth race

With RHEL 6.2, virDomainBlockPull(dom, dev, bandwidth, 0) has a race
with non-zero bandwidth: there is a window between the block_stream
and block_job_set_speed monitor commands where an unlimited amount
of data was let through, defeating the point of a throttle.

This race was first identified in commit a9d3495e, and libvirt was
able to reduce the size of the window for that race.  In the meantime,
the qemu developers decided to fix things properly; per this message:
https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03793.html
the fix will be in qemu 1.1, and changes block-job-set-speed to use
a different parameter name, as well as adding a new optional parameter
to block-stream, which eliminates the race altogether.

Since our documentation already mentioned that we can refuse a non-zero
bandwidth for some hypervisors, I think the best solution is to do
just that for RHEL 6.2 qemu, so that the race is obvious to the user
(anyone using stock RHEL 6.2 binaries won't have this patch, and anyone
building their own libvirt with this patch for RHEL can also rebuild
qemu to get the modern semantics, so it is no real loss in behavior).

Meanwhile the code must be fixed to honor actual qemu 1.1 naming.
Rename the parameter to 'modern', since the naming difference now
covers more than just 'async' block-job-cancel.  And while at it,
fix an unchecked integer overflow.

* src/qemu/qemu_monitor.h (enum BLOCK_JOB_CMD): Drop unused value,
rename enum to match conventions.
* src/qemu/qemu_monitor.c (qemuMonitorBlockJob): Reflect enum rename.
* src/qemu_qemu_monitor_json.h (qemuMonitorJSONBlockJob): Likewise.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Likewise,
and support difference between RHEL 6.2 and qemu 1.1 block pull.
* src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Reject
bandwidth during pull with too-old qemu.
* src/libvirt.c (virDomainBlockPull, virDomainBlockRebase):
Document this.

diff --git a/src/libvirt.c b/src/libvirt.c
index b01ebbaa7b87be28c33817fb23c03a2347d86237..cfd77114a07f7059db699de165542537082d095f 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -18145,7 +18145,9 @@ error:
  * The maximum bandwidth (in Mbps) that will be used to do the copy can be
  * specified with the bandwidth parameter.  If set to 0, libvirt will choose a
  * suitable default.  Some hypervisors do not support this feature and will
- * return an error if bandwidth is not 0.
+ * return an error if bandwidth is not 0; in this case, it might still be
+ * possible for a later call to virDomainBlockJobSetSpeed() to succeed.
+ * The actual speed can be determined with virDomainGetBlockJobInfo().
  *
  * This is shorthand for virDomainBlockRebase() with a NULL base.
  *
@@ -18263,7 +18265,9 @@ error:
  * The maximum bandwidth (in Mbps) that will be used to do the copy can be
  * specified with the bandwidth parameter.  If set to 0, libvirt will choose a
  * suitable default.  Some hypervisors do not support this feature and will
- * return an error if bandwidth is not 0.
+ * return an error if bandwidth is not 0; in this case, it might still be
+ * possible for a later call to virDomainBlockJobSetSpeed() to succeed.
+ * The actual speed can be determined with virDomainGetBlockJobInfo().
  *
  * When @base is NULL and @flags is 0, this is identical to
  * virDomainBlockPull().

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ce31e0928defb282a2b2f1305e0bd57d77683ab4..349274b2518b4839ea4ed50d4c62fc03003e8998 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11667,6 +11667,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
                         _("partial block pull not supported with this "
                           "QEMU binary"));
         goto cleanup;
+    } else if (mode == BLOCK_JOB_PULL && bandwidth) {
+        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                        _("setting bandwidth at start of block pull not "
+                          "supported with this QEMU binary"));
+        goto cleanup;
     }
 
     device = qemuDiskPathToAlias(vm, path, &idx);
@@ -11689,9 +11694,6 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
      * relying on qemu to do this.  */
     ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode,
                               async);
-    if (ret == 0 && mode == BLOCK_JOB_PULL && bandwidth)
-        ret = qemuMonitorBlockJob(priv->mon, device, NULL, bandwidth, NULL,
-                                  BLOCK_JOB_SPEED_INTERNAL, async);
     qemuDomainObjExitMonitorWithDriver(driver, vm);
     if (ret < 0)
         goto endjob;

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 2f66c46a253b81eba67794b658bb54fe05f12c9f..ca1c7aa9103de674557337ac7882da25ed3353af 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2768,23 +2768,34 @@ int qemuMonitorScreendump(qemuMonitorPtr mon,
     return ret;
 }
 
+/* bandwidth is in MB/sec */
 int qemuMonitorBlockJob(qemuMonitorPtr mon,
                         const char *device,
                         const char *base,
                         unsigned long bandwidth,
                         virDomainBlockJobInfoPtr info,
-                        int mode,
-                        bool async)
+                        qemuMonitorBlockJobCmd mode,
+                        bool modern)
 {
     int ret = -1;
+    unsigned long long speed;
 
-    VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o, "
-              "async=%d", mon, device, NULLSTR(base), bandwidth, info, mode,
-              async);
+    VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%luM, info=%p, mode=%o, "
+              "modern=%d", mon, device, NULLSTR(base), bandwidth, info, mode,
+              modern);
+
+    /* Convert bandwidth MiB to bytes */
+    if (bandwidth > ULLONG_MAX / 1024 / 1024) {
+        qemuReportError(VIR_ERR_OVERFLOW,
+                        _("bandwidth must be less than %llu"),
+                        ULLONG_MAX / 1024 / 1024);
+        return -1;
+    }
+    speed = bandwidth * 1024ULL * 1024ULL;
 
     if (mon->json)
-        ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode,
-                                      async);
+        ret = qemuMonitorJSONBlockJob(mon, device, base, speed, info, mode,
+                                      modern);
     else
         qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                         _("block jobs require JSON monitor"));

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index f3cdcddb43d314488bb6d099c468ccdf2eacc35c..ffe8fe7ff99ac49dd35d8d6cf245c21a5f6a97f1 100644 (file)
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -527,20 +527,19 @@ int qemuMonitorSendKey(qemuMonitorPtr mon,
                        unsigned int nkeycodes);
 
 typedef enum {
-    BLOCK_JOB_ABORT = 0,
-    BLOCK_JOB_INFO = 1,
-    BLOCK_JOB_SPEED = 2,
-    BLOCK_JOB_SPEED_INTERNAL = 3,
-    BLOCK_JOB_PULL = 4,
-} BLOCK_JOB_CMD;
+    BLOCK_JOB_ABORT,
+    BLOCK_JOB_INFO,
+    BLOCK_JOB_SPEED,
+    BLOCK_JOB_PULL,
+} qemuMonitorBlockJobCmd;
 
 int qemuMonitorBlockJob(qemuMonitorPtr mon,
                         const char *device,
                         const char *back,
                         unsigned long bandwidth,
                         virDomainBlockJobInfoPtr info,
-                        int mode,
-                        bool async)
+                        qemuMonitorBlockJobCmd mode,
+                        bool modern)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 int qemuMonitorOpenGraphics(qemuMonitorPtr mon,

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index eb58f138045a636fab9e7164be166babbbca9ea8..e1f54532dacdf6f0685e4297aaec4790e5ee6123 100644 (file)
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3423,29 +3423,36 @@ static int qemuMonitorJSONGetBlockJobInfo(virJSONValuePtr reply,
 }
 
 
+/* speed is in bytes/sec */
 int
 qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
                         const char *device,
                         const char *base,
-                        unsigned long bandwidth,
+                        unsigned long long speed,
                         virDomainBlockJobInfoPtr info,
-                        int mode,
-                        bool async)
+                        qemuMonitorBlockJobCmd mode,
+                        bool modern)
 {
     int ret = -1;
     virJSONValuePtr cmd = NULL;
     virJSONValuePtr reply = NULL;
     const char *cmd_name = NULL;
 
-    if (base && (mode != BLOCK_JOB_PULL || !async)) {
+    if (base && (mode != BLOCK_JOB_PULL || !modern)) {
         qemuReportError(VIR_ERR_INTERNAL_ERROR,
                         _("only modern block pull supports base: %s"), base);
         return -1;
     }
+    if (speed && mode == BLOCK_JOB_PULL && !modern) {
+        qemuReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("only modern block pull supports speed: %llu"),
+                        speed);
+        return -1;
+    }
 
-    switch ((BLOCK_JOB_CMD) mode) {
+    switch (mode) {
     case BLOCK_JOB_ABORT:
-        cmd_name = async ? "block-job-cancel" : "block_job_cancel";
+        cmd_name = modern ? "block-job-cancel" : "block_job_cancel";
         cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL);
         break;
     case BLOCK_JOB_INFO:
@@ -3453,19 +3460,24 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
         cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL);
         break;
     case BLOCK_JOB_SPEED:
-    case BLOCK_JOB_SPEED_INTERNAL:
-        cmd_name = async ? "block-job-set-speed" : "block_job_set_speed";
-        cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
-                                         device, "U:value",
-                                         bandwidth * 1024ULL * 1024ULL,
-                                         NULL);
+        cmd_name = modern ? "block-job-set-speed" : "block_job_set_speed";
+        cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device,
+                                         modern ? "U:speed" : "U:value",
+                                         speed, NULL);
         break;
     case BLOCK_JOB_PULL:
-        cmd_name = async ? "block-stream" : "block_stream";
-        cmd = qemuMonitorJSONMakeCommand(cmd_name,
-                                         "s:device", device,
-                                         base ? "s:base" : NULL, base,
-                                         NULL);
+        cmd_name = modern ? "block-stream" : "block_stream";
+        if (speed)
+            cmd = qemuMonitorJSONMakeCommand(cmd_name,
+                                             "s:device", device,
+                                             "U:speed", speed,
+                                             base ? "s:base" : NULL, base,
+                                             NULL);
+        else
+            cmd = qemuMonitorJSONMakeCommand(cmd_name,
+                                             "s:device", device,
+                                             base ? "s:base" : NULL, base,
+                                             NULL);
         break;
     }
 
@@ -3477,14 +3489,9 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
     if (ret == 0 && virJSONValueObjectHasKey(reply, "error")) {
         ret = -1;
         if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) {
-            /* If a job completes before we get a chance to set the
-             * speed, we don't want to fail the original command.  */
-            if (mode == BLOCK_JOB_SPEED_INTERNAL)
-                ret = 0;
-            else
-                qemuReportError(VIR_ERR_OPERATION_INVALID,
-                                _("No active operation on device: %s"),
-                                device);
+            qemuReportError(VIR_ERR_OPERATION_INVALID,
+                            _("No active operation on device: %s"),
+                            device);
         } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")){
             qemuReportError(VIR_ERR_OPERATION_FAILED,
                             _("Device %s in use"), device);
@@ -3497,7 +3504,7 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
                             _("Command '%s' is not found"), cmd_name);
         } else {
             qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                _("Unexpected error"));
+                            _("Unexpected error"));
         }
     }
 

diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index aacbb5ff9fda74af96d35ac7633fc52fd337d349..22a3adff5bb4502e01225c704183795392cb8592 100644 (file)
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -251,10 +251,10 @@ int qemuMonitorJSONScreendump(qemuMonitorPtr mon,
 int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
                             const char *device,
                             const char *base,
-                            unsigned long bandwidth,
+                            unsigned long long speed,
                             virDomainBlockJobInfoPtr info,
-                            int mode,
-                            bool async)
+                            qemuMonitorBlockJobCmd mode,
+                            bool modern)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
 int qemuMonitorJSONSetLink(qemuMonitorPtr mon,