[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 09/11] qemu: blockPull: Refactor the rest of qemuDomainBlockJobImpl




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,
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]