[libvirt] [PATCH 0/3] qemu: Remove support for downstream-only blockjob

See patch 1/3 Peter Krempa (3): qemu: Remove support for legacy block jobs qemu: monitor: Remove support for "legacy" block jobs qemu: caps: Deprecate QEMU_CAPS_BLOCKJOB_SYNC src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_capabilities.h | 2 +- src/qemu/qemu_domain.c | 13 +++------- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 61 +++++++++++--------------------------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 24 ++++++++--------- src/qemu/qemu_monitor.h | 9 +++---- src/qemu/qemu_monitor_json.c | 17 +++++------- src/qemu/qemu_monitor_json.h | 9 +++---- 10 files changed, 44 insertions(+), 96 deletions(-) -- 2.14.1

Block job QMP commands with underscores rather than dashes were never released in upstream qemu, (they were added, but modified in the same release [1]), but a certain distro managed to backport the version in the middle. The change also slightly modified semantics for the abort command, which made us have a lot of code which was only ever present in certain downstream distros. Clean the upstream code from the legacy cruft and support only the upstream implementations. [1] See qemu commit v1.0-2176-gdb58f9c060 --- src/qemu/qemu_domain.c | 13 +++-------- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 60 +++++++++++++------------------------------------- 3 files changed, 19 insertions(+), 56 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 664366b9d..05f8e9488 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6434,29 +6434,22 @@ qemuDomainGetMonitor(virDomainObjPtr vm) /** * qemuDomainSupportsBlockJobs: * @vm: domain object - * @modern: pointer to bool that returns whether modern block jobs are supported * * Returns -1 in case when qemu does not support block jobs at all. Otherwise - * returns 0 and optionally fills @modern to denote that modern (async) block - * jobs are supported. + * returns 0. */ int -qemuDomainSupportsBlockJobs(virDomainObjPtr vm, - bool *modern) +qemuDomainSupportsBlockJobs(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; bool asynchronous = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC); - bool synchronous = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC); - if (!synchronous && !asynchronous) { + if (!asynchronous) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("block jobs not supported with this QEMU binary")); return -1; } - if (modern) - *modern = asynchronous; - return 0; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 468308e4b..b291dc308 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -715,7 +715,7 @@ int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); -int qemuDomainSupportsBlockJobs(virDomainObjPtr vm, bool *modern) +int qemuDomainSupportsBlockJobs(virDomainObjPtr vm) ATTRIBUTE_NONNULL(1); bool qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk); bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool copy_only) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e95683965..c7d93dcb2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16413,7 +16413,6 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; - bool modern; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; @@ -16438,25 +16437,9 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, goto endjob; } - if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) + if (qemuDomainSupportsBlockJobs(vm) < 0) goto endjob; - if (!modern) { - if (base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("partial block pull not supported with this " - "QEMU binary")); - goto endjob; - } - - if (bandwidth) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("setting bandwidth at start of block pull not " - "supported with this QEMU binary")); - goto endjob; - } - } - if (!(disk = qemuDomainDiskByName(vm->def, path))) goto endjob; @@ -16511,7 +16494,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, baseSource); if (!baseSource || basePath) ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath, - speed, modern); + speed, true); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; @@ -16542,7 +16525,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, virDomainDiskDefPtr disk = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool save = false; - bool modern; bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); virDomainObjPtr vm; @@ -16566,7 +16548,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; } - if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) + if (qemuDomainSupportsBlockJobs(vm) < 0) goto endjob; if (!(disk = qemuDomainDiskByName(vm->def, path))) @@ -16583,7 +16565,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; } - if (modern && !async) + if (!async) qemuBlockJobSyncBegin(disk); if (pivot) { @@ -16596,7 +16578,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, } qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, modern); + ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, true); if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; goto endjob; @@ -16623,25 +16605,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom, * while still holding the VM job, to prevent newly scheduled * block jobs from confusing us. */ if (!async) { - if (!modern) { - /* Older qemu that lacked async reporting also lacked - * blockcopy and active commit, so we can hardcode the - * event to pull and let qemuBlockJobEventProcess() handle - * the rest as usual */ - qemuBlockJobEventProcess(driver, vm, disk, - QEMU_ASYNC_JOB_NONE, - VIR_DOMAIN_BLOCK_JOB_TYPE_PULL, - VIR_DOMAIN_BLOCK_JOB_CANCELED); - } else { - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk); - while (diskPriv->blockjob) { - if (virDomainObjWait(vm) < 0) { - ret = -1; - goto endjob; - } - qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk); + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk); + while (diskPriv->blockjob) { + if (virDomainObjWait(vm) < 0) { + ret = -1; + goto endjob; } + qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk); } } @@ -16728,7 +16699,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, goto endjob; } - if (qemuDomainSupportsBlockJobs(vm, NULL) < 0) + if (qemuDomainSupportsBlockJobs(vm) < 0) goto endjob; if (!(disk = virDomainDiskByName(vm->def, path, true))) { @@ -16784,7 +16755,6 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, virDomainDiskDefPtr disk; int ret = -1; virDomainObjPtr vm; - bool modern; const char *device; unsigned long long speed = bandwidth; @@ -16816,7 +16786,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, goto endjob; } - if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) + if (qemuDomainSupportsBlockJobs(vm) < 0) goto endjob; if (!(disk = qemuDomainDiskByName(vm->def, path))) @@ -16829,7 +16799,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, ret = qemuMonitorBlockJobSetSpeed(qemuDomainGetMonitor(vm), device, speed, - modern); + true); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; -- 2.14.1

On 09/13/2017 08:52 AM, Peter Krempa wrote:
Block job QMP commands with underscores rather than dashes were never released in upstream qemu, (they were added, but modified in the same release [1]), but a certain distro managed to backport the version in the middle.
I find it fine that you don't call out the "certain distro" by name in the commit message - but several readers can probably already guess, so I'm not spilling the beans further if I clear my throat a bit "<cough>RHEL 6</cough>". Let's visit the consequences of this patch: If downstream Red Hat wants to rebase to a version of libvirt containing this patch, that downstream version will HAVE to use the modern spelling only - but presumably that rebase will only be done in tandem with a new enough qemu that also supplies the modern spelling. Meanwhile, when it comes to supporting loooong lifecycles of older releases, downstream has already picked the version of libvirt they intend to maintain forever, and will NOT be rebasing either libvirt or qemu, so they won't be missing anything dropped in this patch because they won't be using this patch. So I welcome this cleanup.
The change also slightly modified semantics for the abort command, which made us have a lot of code which was only ever present in certain downstream distros.
Clean the upstream code from the legacy cruft and support only the upstream implementations.
[1] See qemu commit v1.0-2176-gdb58f9c060 --- src/qemu/qemu_domain.c | 13 +++-------- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 60 +++++++++++++------------------------------------- 3 files changed, 19 insertions(+), 56 deletions(-)
@@ -16511,7 +16494,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, baseSource); if (!baseSource || basePath) ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath, - speed, modern); + speed, true);
Presumably a later patch cleans this up further? Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

Drop all the monitor code necessary to do the downstream block jobs. --- src/qemu/qemu_driver.c | 7 +++---- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 24 ++++++++++-------------- src/qemu/qemu_monitor.h | 9 +++------ src/qemu/qemu_monitor_json.c | 17 +++++++---------- src/qemu/qemu_monitor_json.h | 9 +++------ 6 files changed, 27 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c7d93dcb2..b334cf20b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16494,7 +16494,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, baseSource); if (!baseSource || basePath) ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath, - speed, true); + speed); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; @@ -16578,7 +16578,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, } qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, true); + ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device); if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; goto endjob; @@ -16798,8 +16798,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJobSetSpeed(qemuDomainGetMonitor(vm), device, - speed, - true); + speed); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 272d525f1..492815b60 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -762,7 +762,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; - rv = qemuMonitorBlockJobCancel(priv->mon, diskAlias, true); + rv = qemuMonitorBlockJobCancel(priv->mon, diskAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0) goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 19082d8bf..69f14d028 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3475,43 +3475,39 @@ qemuMonitorBlockStream(qemuMonitorPtr mon, const char *device, const char *base, const char *backingName, - unsigned long long bandwidth, - bool modern) + unsigned long long bandwidth) { - VIR_DEBUG("device=%s, base=%s, backingName=%s, bandwidth=%lluB, modern=%d", - device, NULLSTR(base), NULLSTR(backingName), bandwidth, modern); + VIR_DEBUG("device=%s, base=%s, backingName=%s, bandwidth=%lluB", + device, NULLSTR(base), NULLSTR(backingName), bandwidth); QEMU_CHECK_MONITOR_JSON(mon); - return qemuMonitorJSONBlockStream(mon, device, base, backingName, - bandwidth, modern); + return qemuMonitorJSONBlockStream(mon, device, base, backingName, bandwidth); } int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, - const char *device, - bool modern) + const char *device) { - VIR_DEBUG("device=%s, modern=%d", device, modern); + VIR_DEBUG("device=%s", device); QEMU_CHECK_MONITOR_JSON(mon); - return qemuMonitorJSONBlockJobCancel(mon, device, modern); + return qemuMonitorJSONBlockJobCancel(mon, device); } int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, - unsigned long long bandwidth, - bool modern) + unsigned long long bandwidth) { - VIR_DEBUG("device=%s, bandwidth=%lluB, modern=%d", device, bandwidth, modern); + VIR_DEBUG("device=%s, bandwidth=%lluB", device, bandwidth); QEMU_CHECK_MONITOR_JSON(mon); - return qemuMonitorJSONBlockJobSetSpeed(mon, device, bandwidth, modern); + return qemuMonitorJSONBlockJobSetSpeed(mon, device, bandwidth); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 9805a3390..6414d2483 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -896,19 +896,16 @@ int qemuMonitorBlockStream(qemuMonitorPtr mon, const char *device, const char *base, const char *backingName, - unsigned long long bandwidth, - bool modern) + unsigned long long bandwidth) ATTRIBUTE_NONNULL(2); int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, - const char *device, - bool modern) + const char *device) ATTRIBUTE_NONNULL(2); int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, - unsigned long long bandwidth, - bool modern); + unsigned long long bandwidth); typedef struct _qemuMonitorBlockJobInfo qemuMonitorBlockJobInfo; typedef qemuMonitorBlockJobInfo *qemuMonitorBlockJobInfoPtr; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index df5fb7c8f..0fc5d2122 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4474,13 +4474,12 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon, const char *device, const char *base, const char *backingName, - unsigned long long speed, - bool modern) + unsigned long long speed) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - const char *cmd_name = modern ? "block-stream" : "block_stream"; + const char *cmd_name = "block-stream"; if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, @@ -4507,13 +4506,12 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon, int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, - const char *device, - bool modern) + const char *device) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - const char *cmd_name = modern ? "block-job-cancel" : "block_job_cancel"; + const char *cmd_name = "block-job-cancel"; if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, @@ -4538,17 +4536,16 @@ qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, - unsigned long long speed, - bool modern) + unsigned long long speed) { int ret = -1; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; - const char *cmd_name = modern ? "block-job-set-speed" : "block_job_set_speed"; + const char *cmd_name = "block-job-set-speed"; if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, - modern ? "J:speed" : "J:value", speed, + "J:speed", speed, NULL))) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 7462967b5..0cdfc5ead 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -300,19 +300,16 @@ int qemuMonitorJSONBlockStream(qemuMonitorPtr mon, const char *device, const char *base, const char *backingName, - unsigned long long speed, - bool modern) + unsigned long long speed) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, - const char *device, - bool modern) + const char *device) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, - unsigned long long speed, - bool modern) + unsigned long long speed) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); virHashTablePtr qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon) -- 2.14.1

On 09/13/2017 08:52 AM, Peter Krempa wrote:
Drop all the monitor code necessary to do the downstream block jobs. --- src/qemu/qemu_driver.c | 7 +++---- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 24 ++++++++++-------------- src/qemu/qemu_monitor.h | 9 +++------ src/qemu/qemu_monitor_json.c | 17 +++++++---------- src/qemu/qemu_monitor_json.h | 9 +++------ 6 files changed, 27 insertions(+), 41 deletions(-)
Yep, resolves my question on 1/3. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

Interrestingly, none of the qemus we have caps for supported it ... --- src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_capabilities.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e7ea6f47c..c690cb349 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1562,7 +1562,6 @@ struct virQEMUCapsStringFlags { struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "system_wakeup", QEMU_CAPS_WAKEUP }, { "transaction", QEMU_CAPS_TRANSACTION }, - { "block_stream", QEMU_CAPS_BLOCKJOB_SYNC }, { "block-stream", QEMU_CAPS_BLOCKJOB_ASYNC }, { "dump-guest-memory", QEMU_CAPS_DUMP_GUEST_MEMORY }, { "query-spice", QEMU_CAPS_SPICE }, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f32687d4a..85c390abf 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -174,7 +174,7 @@ typedef enum { QEMU_CAPS_TRANSACTION, /* transaction monitor command */ /* 90 */ - QEMU_CAPS_BLOCKJOB_SYNC, /* old block_job_cancel, block_stream */ + X_QEMU_CAPS_BLOCKJOB_SYNC, /* old block_job_cancel, block_stream */ QEMU_CAPS_BLOCKJOB_ASYNC, /* new block-job-cancel, block-stream */ QEMU_CAPS_SCSI_CD, /* -device scsi-cd */ QEMU_CAPS_IDE_CD, /* -device ide-cd */ -- 2.14.1

On 09/13/2017 08:52 AM, Peter Krempa wrote:
Interrestingly, none of the qemus we have caps for supported it ...
s/Interrestingly/Interestingly/
--- src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_capabilities.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake
-
Peter Krempa