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 077c161..425d340 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11650,14 +11650,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)) {
while (1) {
/* Poll every 50ms */
@@ -11734,15 +11740,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 7c893d4..aea3765 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3438,7 +3438,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",
@@ -3460,22 +3460,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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list