On 04/01/2015 01:20 PM, Peter Krempa wrote:
> Since it now handles only block pull code paths we can refactor it and
> remove tons of cruft.
> ---
> src/qemu/qemu_driver.c | 86 ++++++++++++++++++++------------------------
> src/qemu/qemu_monitor.c | 30 ++++++++--------
> src/qemu/qemu_monitor.h | 17 ++++-----
> src/qemu/qemu_monitor_json.c | 59 +++++++-----------------------
> src/qemu/qemu_monitor_json.h | 13 ++++---
> 5 files changed, 78 insertions(+), 127 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7b7775d..2dd8ed4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16162,17 +16162,16 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
> /* bandwidth in MiB/s per public API. Caller must lock vm beforehand,
> * and not access it afterwards. */
> static int
> -qemuDomainBlockJobImpl(virDomainObjPtr vm,
> - virConnectPtr conn,
> - const char *path, const char *base,
> - unsigned long bandwidth,
> - int mode, unsigned int flags)
> +qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + const char *path,
> + const char *base,
> + unsigned long bandwidth,
> + unsigned int flags)
> {
> - virQEMUDriverPtr driver = conn->privateData;
> - qemuDomainObjPrivatePtr priv;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> char *device = NULL;
> - int ret = -1;
> - bool async = false;
> + bool modern;
> int idx;
> virDomainDiskDefPtr disk;
> virStorageSourcePtr baseSource = NULL;
> @@ -16180,12 +16179,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> char *basePath = NULL;
> char *backingPath = NULL;
> unsigned long long speed = bandwidth;
> -
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("domain is not running"));
> - goto cleanup;
> - }
> + int ret = -1;
>
> if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -16194,23 +16188,23 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> goto cleanup;
> }
>
> - priv = vm->privateData;
> - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) {
> - async = true;
> - } else if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("block jobs not supported with this QEMU binary"));
> - goto cleanup;
> - } else if (base) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("partial block pull not supported with this "
> - "QEMU binary"));
> - goto cleanup;
> - } else if (mode == BLOCK_JOB_PULL && bandwidth) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("setting bandwidth at start of block pull not "
> - "supported with this QEMU binary"));
> + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0)
> goto cleanup;
> +
> + if (!modern) {
> + if (base) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("partial block pull not supported with this "
> + "QEMU binary"));
> + goto cleanup;
> + }
> +
> + if (bandwidth) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("setting bandwidth at start of block pull not "
> + "supported with this QEMU binary"));
> + goto cleanup;
> + }
> }
>
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> @@ -16222,12 +16216,11 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> goto endjob;
> }
>
> - device = qemuDiskPathToAlias(vm, path, &idx);
> - if (!device)
> + if (!(device = qemuDiskPathToAlias(vm, path, &idx)))
> goto endjob;
> disk = vm->def->disks[idx];
>
> - if (mode == BLOCK_JOB_PULL && qemuDomainDiskBlockJobIsActive(disk))
> + if (qemuDomainDiskBlockJobIsActive(disk))
> goto endjob;
>
> if (base &&
> @@ -16250,7 +16243,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> &backingPath) < 0)
> goto endjob;
>
> -
> if (!backingPath) {
> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("can't keep relative backing relationship"));
> @@ -16260,8 +16252,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> }
>
> /* Convert bandwidth MiB to bytes, if needed */
> - if (mode == BLOCK_JOB_PULL &&
> - !(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES)) {
> + if (!(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES)) {
> if (speed > LLONG_MAX >> 20) {
> virReportError(VIR_ERR_OVERFLOW,
> _("bandwidth must be less than %llu"),
> @@ -16276,15 +16267,15 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
> baseSource);
> if (!baseSource || basePath)
> - ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
> - speed, mode, async);
> + ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath,
> + speed, modern);
These don't use your new qemuDomainGetMonitor(vm) - not that it really
matters, but since you created it...
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> ret = -1;
> - if (ret < 0) {
> +
> + if (ret < 0)
> goto endjob;
> - } else if (mode == BLOCK_JOB_PULL) {
> - disk->blockjob = true;
> - }
> +
> + disk->blockjob = true;
>
> endjob:
> qemuDomainObjEndJob(driver, vm);
> @@ -16297,6 +16288,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> return ret;
> }
>
> +
> static int
> qemuDomainBlockJobAbort(virDomainPtr dom,
> const char *path,
> @@ -16778,6 +16770,7 @@ static int
> qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
> unsigned long bandwidth, unsigned int flags)
> {
> + virQEMUDriverPtr driver = dom->conn->privateData;
> virDomainObjPtr vm;
> int ret = -1;
> unsigned long long speed = bandwidth;
> @@ -16800,8 +16793,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
> /* For normal rebase (enhanced blockpull), the common code handles
> * everything, including vm cleanup. */
> if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY))
> - return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth,
> - BLOCK_JOB_PULL, flags);
> + return qemuDomainBlockPullCommon(driver, vm, path, base, bandwidth, flags);
>
> /* If we got here, we are doing a block copy rebase. */
> if (VIR_ALLOC(dest) < 0)
> @@ -16934,8 +16926,8 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth,
> return -1;
> }
>
> - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth,
> - BLOCK_JOB_PULL, flags);
> + return qemuDomainBlockPullCommon(dom->conn->privateData,
> + vm, path, NULL, bandwidth, flags);
> }
>
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index e413d91..4791029 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3577,28 +3577,26 @@ int qemuMonitorScreendump(qemuMonitorPtr mon,
>
> /* bandwidth is in bytes/sec */
> int
> -qemuMonitorBlockJob(qemuMonitorPtr mon,
> - const char *device,
> - const char *base,
> - const char *backingName,
> - unsigned long long bandwidth,
> - qemuMonitorBlockJobCmd mode,
> - bool modern)
> +qemuMonitorBlockStream(qemuMonitorPtr mon,
> + const char *device,
> + const char *base,
> + const char *backingName,
> + unsigned long long bandwidth,
> + bool modern)
> {
> - int ret = -1;
> -
> VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%lluB, "
> - "mode=%o, modern=%d",
> + "modern=%d",
> mon, device, NULLSTR(base), NULLSTR(backingName),
> - bandwidth, mode, modern);
> + bandwidth, modern);
>
> - if (mon->json)
> - ret = qemuMonitorJSONBlockJob(mon, device, base, backingName,
> - bandwidth, mode, modern);
> - else
> + if (!mon->json) {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> _("block jobs require JSON monitor"));
> - return ret;
> + return -1;
> + }
> +
> + return qemuMonitorJSONBlockStream(mon, device, base, backingName,
> + bandwidth, modern);
> }
>
>
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 5cc811a..d673154 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -755,17 +755,12 @@ int qemuMonitorSendKey(qemuMonitorPtr mon,
> unsigned int *keycodes,
> unsigned int nkeycodes);
>
> -typedef enum {
> - BLOCK_JOB_PULL,
> -} qemuMonitorBlockJobCmd;
> -
> -int qemuMonitorBlockJob(qemuMonitorPtr mon,
> - const char *device,
> - const char *base,
> - const char *backingName,
> - unsigned long long bandwidth,
> - qemuMonitorBlockJobCmd mode,
> - bool modern)
> +int qemuMonitorBlockStream(qemuMonitorPtr mon,
> + const char *device,
> + const char *base,
> + const char *backingName,
> + unsigned long long bandwidth,
> + bool modern)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> int qemuMonitorBlockJobCancel(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 01a4f9a..d02567d 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4286,57 +4286,24 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply,
>
> /* speed is in bytes/sec */
> int
> -qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
> - const char *device,
> - const char *base,
> - const char *backingName,
> - unsigned long long speed,
> - qemuMonitorBlockJobCmd mode,
> - bool modern)
> +qemuMonitorJSONBlockStream(qemuMonitorPtr mon,
> + const char *device,
> + const char *base,
> + const char *backingName,
> + unsigned long long speed,
> + bool modern)
> {
> int ret = -1;
> virJSONValuePtr cmd = NULL;
> virJSONValuePtr reply = NULL;
> - const char *cmd_name = NULL;
> -
> - if (base && (mode != BLOCK_JOB_PULL || !modern)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("only modern block pull supports base: %s"), base);
> - return -1;
> - }
Just checking...
This change is essentially the same as in qemuDomainBlockPullCommon
where if (!modern) {} was added right?
> -
> - if (backingName && mode != BLOCK_JOB_PULL) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("backing name is supported only for block pull"));
> - return -1;
> - }
And this won't be necessary.... since we no longer have multiple
(ab)users of the same API
> -
> - if (backingName && !base) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("backing name requires a base image"));
> - return -1;
> - }
Is there a check for this somewhere that I missed?
> + const char *cmd_name = modern ? "block-stream" : "block_stream";
>
> - if (speed && mode == BLOCK_JOB_PULL && !modern) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("only modern block pull supports speed: %llu"),
> - speed);
> - return -1;
> - }
And this is the second half of the check in qemuDomainBlockPullCommon
ACK - in general - Just want to make sure the "if (backingName && !base)
wasn't erroneously removed.
John
> -
> - switch (mode) {
> - case BLOCK_JOB_PULL:
> - cmd_name = modern ? "block-stream" : "block_stream";
> - cmd = qemuMonitorJSONMakeCommand(cmd_name,
> - "s:device", device,
> - "Y:speed", speed,
> - "S:base", base,
> - "S:backing-file", backingName,
> - NULL);
> - break;
> - }
> -
> - if (!cmd)
> + if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name,
> + "s:device", device,
> + "Y:speed", speed,
> + "S:base", base,
> + "S:backing-file", backingName,
> + NULL)))
> return -1;
>
> if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index c88972c..43adc90 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -297,13 +297,12 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon,
> int qemuMonitorJSONScreendump(qemuMonitorPtr mon,
> const char *file);
>
> -int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
> - const char *device,
> - const char *base,
> - const char *backingName,
> - unsigned long long speed,
> - qemuMonitorBlockJobCmd mode,
> - bool modern)
> +int qemuMonitorJSONBlockStream(qemuMonitorPtr mon,
> + const char *device,
> + const char *base,
> + const char *backingName,
> + unsigned long long speed,
> + bool modern)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list