On Wed, Apr 11, 2012 at 05:40:34PM -0600, Eric Blake wrote: > RHEL 6.2 was released with an early version of block jobs, which only > worked on the qed file format, where the commands were spelled with > underscore (contrary to QMP style), and where 'block_job_cancel' was > synchronous and did not trigger an event. > > qemu 1.1 has fixed these short-comings [1][2]: the commands now work on > multiple file types, are spelled with dash, and 'block-job-cancel' is > asynchronous and emits an event upon conclusion. > > [1]qemu commit 370521a1d6f5537ea7271c119f3fbb7b0fa57063 > [2]https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01248.html > > This patch recognizes the new spellings, and fixes virDomainBlockRebase > to give a graceful error when talking to a too-old qemu on a partial > rebase attempt. > > * src/qemu/qemu_capabilities.h (QEMU_CAPS_BLOCKJOB_SYNC) > (QEMU_CAPS_BLOCKJOB_ASYNC): New bits. > * src/qemu/qemu_capabilities.c (qemuCaps): Name them. > * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set > them. > (qemuMonitorJSONBlockJob): Manage both command names. > (qemuMonitorJSONDiskSnapshot): Minor formatting fix. > * src/qemu/qemu_monitor.h (qemuMonitorBlockJob): Alter signature. > * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockJob): Likewise. > * src/qemu/qemu_monitor.c (qemuMonitorBlockJob): Pass through > capability bit. > * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Update callers. > --- > src/qemu/qemu_capabilities.c | 3 ++ > src/qemu/qemu_capabilities.h | 2 + > src/qemu/qemu_driver.c | 22 ++++++++++++++---- > src/qemu/qemu_monitor.c | 11 +++++--- > src/qemu/qemu_monitor.h | 5 ++- > src/qemu/qemu_monitor_json.c | 51 +++++++++++++++++++++++------------------- > src/qemu/qemu_monitor_json.h | 4 ++- > 7 files changed, 63 insertions(+), 35 deletions(-) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 0e09d6d..fd4ca4d 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -156,6 +156,9 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, > "scsi-disk.channel", > "scsi-block", > "transaction", > + > + "block-job-sync", /* 90 */ > + "block-job-async", > ); > > struct qemu_feature_flags { > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index 78cdbe0..6550d59 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -124,6 +124,8 @@ enum qemuCapsFlags { > QEMU_CAPS_SCSI_DISK_CHANNEL = 87, /* Is scsi-disk.channel available? */ > QEMU_CAPS_SCSI_BLOCK = 88, /* -device scsi-block */ > QEMU_CAPS_TRANSACTION = 89, /* transaction monitor command */ > + QEMU_CAPS_BLOCKJOB_SYNC = 90, /* RHEL 6.2 block_job_cancel */ > + QEMU_CAPS_BLOCKJOB_ASYNC = 91, /* qemu 1.1 block-job-cancel */ > > QEMU_CAPS_LAST, /* this must always be the last item */ > }; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index e18e72d..f939a31 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -11607,6 +11607,7 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, > char uuidstr[VIR_UUID_STRING_BUFLEN]; > char *device = NULL; > int ret = -1; > + bool async = false; > > qemuDriverLock(driver); > virUUIDFormat(dom->uuid, uuidstr); > @@ -11616,6 +11617,19 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, > _("no domain with matching uuid '%s'"), uuidstr); > goto cleanup; > } > + priv = vm->privateData; > + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { > + async = true; > + } else if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { > + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("block jobs not supported with this QEMU binary")); > + goto cleanup; > + } else if (base) { > + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("partial block pull not supported with this " > + "QEMU binary")); > + goto cleanup; > + } > > device = qemuDiskPathToAlias(vm, path, NULL); > if (!device) { > @@ -11632,13 +11646,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, > } > > qemuDomainObjEnterMonitorWithDriver(driver, vm); > - priv = vm->privateData; > - /* XXX - add a qemu capability check, since only qemu 1.1 or newer > - * supports the base argument. > - * XXX - libvirt should really be tracking the backing file chain > + /* XXX - libvirt should really be tracking the backing file chain > * 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); > + ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode, > + async); > qemuDomainObjExitMonitorWithDriver(driver, vm); > > endjob: > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index e1a8d4c..36f3832 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -2773,15 +2773,18 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, > const char *base, > unsigned long bandwidth, > virDomainBlockJobInfoPtr info, > - int mode) > + int mode, > + bool async) > { > int ret = -1; > > - VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o", > - mon, device, NULLSTR(base), bandwidth, info, mode); > + VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o, " > + "async=%d", mon, device, NULLSTR(base), bandwidth, info, mode, > + async); > > if (mon->json) > - ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode); > + ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode, > + async); > else > qemuReportError(VIR_ERR_INVALID_ARG, "%s", > _("block jobs require JSON monitor")); > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index b480966..dc02964 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -538,8 +538,9 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, > const char *back, > unsigned long bandwidth, > virDomainBlockJobInfoPtr info, > - int mode) > - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); > + int mode, > + bool async) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); Hum, gcc wasn't complaining on an "ATTRIBUTE_NONNULL(5)" for something which wasn't a pointer ? > int qemuMonitorOpenGraphics(qemuMonitorPtr mon, > const char *protocol, > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index d09d779..d4d2c3e 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -939,12 +939,14 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon, > > if (STREQ(name, "human-monitor-command")) > *json_hmp = 1; > - > - if (STREQ(name, "system_wakeup")) > + else if (STREQ(name, "system_wakeup")) > qemuCapsSet(qemuCaps, QEMU_CAPS_WAKEUP); > - > - if (STREQ(name, "transaction")) > + else if (STREQ(name, "transaction")) > qemuCapsSet(qemuCaps, QEMU_CAPS_TRANSACTION); > + else if (STREQ(name, "block_job_cancel")) > + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC); > + else if (STREQ(name, "block-job-cancel")) > + qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC); > } Hum ... seems to me that we always set bits QEMU_CAPS_BLOCKJOB_SYNC and QEMU_CAPS_BLOCKJOB_ASYNC together, so do you envision cases where one was set and not the other ? If not why not merge them for the sake of one less bit to manage ? > ret = 0; > @@ -3114,7 +3116,7 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, > const char *device, const char *file, > const char *format, bool reuse) > { > - int ret; > + int ret = -1; > virJSONValuePtr cmd; > virJSONValuePtr reply = NULL; > > @@ -3132,14 +3134,13 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, > if (actions) { > if (virJSONValueArrayAppend(actions, cmd) < 0) { > virReportOOMError(); > - ret = -1; > } else { > ret = 0; > cmd = NULL; > } > } else { > if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) > - goto cleanup; > + goto cleanup; > > if (qemuMonitorJSONHasError(reply, "CommandNotFound") && > qemuMonitorCheckHMP(mon, "snapshot_blkdev")) { > @@ -3388,39 +3389,43 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, > const char *base, > unsigned long bandwidth, > virDomainBlockJobInfoPtr info, > - int mode) > + int mode, > + bool async) > { > int ret = -1; > virJSONValuePtr cmd = NULL; > virJSONValuePtr reply = NULL; > const char *cmd_name = NULL; > > - if (base && mode != BLOCK_JOB_PULL) { > + if (base && (mode != BLOCK_JOB_PULL || !async)) { > qemuReportError(VIR_ERR_INTERNAL_ERROR, > - _("only block pull supports base: %s"), base); > + _("only modern block pull supports base: %s"), base); > return -1; > } > > - if (mode == BLOCK_JOB_ABORT) { > - cmd_name = "block_job_cancel"; > + switch ((BLOCK_JOB_CMD) mode) { > + case BLOCK_JOB_ABORT: > + cmd_name = async ? "block-job-cancel" : "block_job_cancel"; > cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL); > - } else if (mode == BLOCK_JOB_INFO) { > + break; > + case BLOCK_JOB_INFO: > cmd_name = "query-block-jobs"; > cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL); > - } else if (mode == BLOCK_JOB_SPEED) { > - cmd_name = "block_job_set_speed"; > + break; > + case BLOCK_JOB_SPEED: > + cmd_name = async ? "block-job-set-speed" : "block_job_set_speed"; > cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", > device, "U:value", > bandwidth * 1024ULL * 1024ULL, > NULL); > - } else if (mode == BLOCK_JOB_PULL) { > - cmd_name = "block_stream"; > - if (base) > - cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", > - device, "s:base", base, NULL); > - else > - cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", > - device, 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); > + break; > } > > if (!cmd) > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index a0f67aa..aacbb5f 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -253,7 +253,9 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, > const char *base, > unsigned long bandwidth, > virDomainBlockJobInfoPtr info, > - int mode); > + int mode, > + bool async) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); > > int qemuMonitorJSONSetLink(qemuMonitorPtr mon, > const char *name, Minor point about the extra bit, to me that's fine, ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list