In my testing, I was able to provoke an odd block pull failure:

$ virsh blockpull dom vda --bandwidth 10000
error: Requested operation is not valid: No active operation on device: 
drive-virtio-disk0

merely by using gdb to artifically wait to do the block job set speed
until after the pull had already finished.  But in reality, that should
be a success, since the pull finished before we had a chance to set
speed.  Furthermore, using a double job lock is annoying; we can alter
the speed as part of the same job that started the block pull for less
likelihood of hitting the speed failure in the first place.

In fixing this, I discovered commit 10ec36e2 was wrong for marking
the info parameter of qemuMonitorBlockJob as non-null.

* src/qemu/qemu_monitor.h (qemuMonitorBlockJob): Fix prototype.
(BLOCK_JOB_CMD): Add new mode.
* src/qemu/qemu_driver.c (qemuDomainBlockRebase): Move secondary
job call...
(qemuDomainBlockJobImpl): ...here, for fewer locks.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Change
return value on new internal mode.
---
 src/qemu/qemu_driver.c       |   18 +++++++++---------
 src/qemu/qemu_monitor.h      |    5 +++--
 src/qemu/qemu_monitor_json.c |   32 ++++++++++++++++++++------------
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5f0eb05..f3bd043 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11639,14 +11639,20 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char 
*path, const char *base,
      * itself, and validating that base is on the chain, rather than
      * relying on qemu to do this.  */
     ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode);
+    if (ret == 0 && mode == BLOCK_JOB_PULL && bandwidth)
+        ret = qemuMonitorBlockJob(priv->mon, device, NULL, bandwidth, NULL,
+                                  BLOCK_JOB_SPEED_INTERNAL);
     qemuDomainObjExitMonitorWithDriver(driver, vm);

+    if (ret < 0)
+        goto endjob;
+
     /* Qemu provides asynchronous block job cancellation, but without
      * the VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC flag libvirt guarantees a
      * synchronous operation.  Provide this behavior by waiting here,
      * so we don't get confused by newly scheduled block jobs.
      */
-    if (ret == 0 && mode == BLOCK_JOB_ABORT &&
+    if (mode == BLOCK_JOB_ABORT &&
         !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
         ret = 1;
         while (1) {
@@ -11724,15 +11730,9 @@ static int
 qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
                       unsigned long bandwidth, unsigned int flags)
 {
-    int ret;
-
     virCheckFlags(0, -1);
-    ret = qemuDomainBlockJobImpl(dom, path, base, bandwidth, NULL,
-                                 BLOCK_JOB_PULL, flags);
-    if (ret == 0 && bandwidth != 0)
-        ret = qemuDomainBlockJobImpl(dom, path, NULL, bandwidth, NULL,
-                                     BLOCK_JOB_SPEED, flags);
-    return ret;
+    return qemuDomainBlockJobImpl(dom, path, base, bandwidth, NULL,
+                                  BLOCK_JOB_PULL, flags);
 }

 static int
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index b480966..2e6ac79 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -530,7 +530,8 @@ typedef enum {
     BLOCK_JOB_ABORT = 0,
     BLOCK_JOB_INFO = 1,
     BLOCK_JOB_SPEED = 2,
-    BLOCK_JOB_PULL = 3,
+    BLOCK_JOB_SPEED_INTERNAL = 3,
+    BLOCK_JOB_PULL = 4,
 } BLOCK_JOB_CMD;

 int qemuMonitorBlockJob(qemuMonitorPtr mon,
@@ -539,7 +540,7 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
                         unsigned long bandwidth,
                         virDomainBlockJobInfoPtr info,
                         int mode)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5);
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

 int qemuMonitorOpenGraphics(qemuMonitorPtr mon,
                             const char *protocol,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index c9f0f0c..13c35c8 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3435,7 +3435,7 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
     } else if (mode == BLOCK_JOB_INFO) {
         cmd_name = "query-block-jobs";
         cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL);
-    } else if (mode == BLOCK_JOB_SPEED) {
+    } else if (mode == BLOCK_JOB_SPEED || mode == BLOCK_JOB_SPEED_INTERNAL) {
         cmd_name = "block_job_set_speed";
         cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
                                          device, "U:value",
@@ -3457,22 +3457,30 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
     ret = qemuMonitorJSONCommand(mon, cmd, &reply);

     if (ret == 0 && virJSONValueObjectHasKey(reply, "error")) {
-        if (qemuMonitorJSONHasError(reply, "DeviceNotActive"))
-            qemuReportError(VIR_ERR_OPERATION_INVALID,
-                _("No active operation on device: %s"), device);
-        else if (qemuMonitorJSONHasError(reply, "DeviceInUse"))
+        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);
+        } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")){
             qemuReportError(VIR_ERR_OPERATION_FAILED,
-                _("Device %s in use"), device);
-        else if (qemuMonitorJSONHasError(reply, "NotSupported"))
+                            _("Device %s in use"), device);
+        } else if (qemuMonitorJSONHasError(reply, "NotSupported")) {
             qemuReportError(VIR_ERR_OPERATION_INVALID,
-                _("Operation is not supported for device: %s"), device);
-        else if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
+                            _("Operation is not supported for device: %s"),
+                            device);
+        } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
             qemuReportError(VIR_ERR_OPERATION_INVALID,
-                _("Command '%s' is not found"), cmd_name);
-        else
+                            _("Command '%s' is not found"), cmd_name);
+        } else {
             qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                 _("Unexpected error"));
-        ret = -1;
+        }
     }

     if (ret == 0 && mode == BLOCK_JOB_INFO)
-- 
1.7.7.6

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

Reply via email to