9:09 a.m.
On Wed, Apr 08, 2015 at 13:10:12 -0400, John Ferlan wrote:
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
...
> @@ -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...
It was meant for functions that don't need the 'priv' variable otherwise
so that it can be avoided. Here 'priv' is needed for checking a
capability so I did not bother changing 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);
...
> 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?
Yes. This one would be redundant.
> -
> - 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
Exactly.
> -
> - 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?
The caller ensures that this does not happen. We could leave this one
possibly in if you want.
> + 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
Peter