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

Re: [libvirt] [PATCHv2 14/16] blockjob: manage qemu block-commit monitor command



On 10/13/2012 06:00 PM, Eric Blake wrote:
> qemu 1.3 will be adding a 'block-commit' monitor command, per
> qemu.git commit ed61fc1.  It matches nicely to the libvirt API
> virDomainBlockCommit.

Hmm. Imagine that. What serendipity. I wonder how that could have
happened. etc.... :-)

>
> * src/qemu/qemu_capabilities.h (QEMU_CAPS_BLOCK_COMMIT): New bit.
> * src/qemu/qemu_capabilities.c (qemuCapsProbeQMPCommands): Set it.
> * src/qemu/qemu_monitor.h (qemuMonitorBlockCommit): New prototype.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockCommit):
> Likewise.
> * src/qemu/qemu_monitor.c (qemuMonitorBlockCommit): Implement it.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockCommit):
> Likewise.
> (qemuMonitorJSONHandleBlockJobImpl)
> (qemuMonitorJSONGetBlockJobInfoOne): Handle new event type.
> ---
>
> Change from v1:
> upstream qemu now has the commit
> recalculate backing chain after commit completes
>
>  src/qemu/qemu_capabilities.c |  3 +++
>  src/qemu/qemu_capabilities.h |  1 +
>  src/qemu/qemu_monitor.c      | 30 ++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor.h      |  7 +++++++
>  src/qemu/qemu_monitor_json.c | 34 ++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_json.h |  7 +++++++
>  src/qemu/qemu_process.c      | 15 +++++++++------
>  7 files changed, 91 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index fb0fe54..73a12f1 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -187,6 +187,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>                "reboot-timeout", /* 110 */
>                "dump-guest-core",
>                "seamless-migration",
> +              "block-commit",
>      );
>
>  struct _qemuCaps {
> @@ -1881,6 +1882,8 @@ qemuCapsProbeQMPCommands(qemuCapsPtr caps,
>              qemuCapsSet(caps, QEMU_CAPS_SPICE);
>          else if (STREQ(name, "query-kvm"))
>              qemuCapsSet(caps, QEMU_CAPS_KVM);
> +        else if (STREQ(name, "block-commit"))
> +            qemuCapsSet(caps, QEMU_CAPS_BLOCK_COMMIT);
>          VIR_FREE(name);
>      }
>      VIR_FREE(commands);
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 5d343c1..6939c45 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -150,6 +150,7 @@ enum qemuCapsFlags {
>      QEMU_CAPS_REBOOT_TIMEOUT     = 110, /* -boot reboot-timeout */
>      QEMU_CAPS_DUMP_GUEST_CORE    = 111, /* dump-guest-core-parameter */
>      QEMU_CAPS_SEAMLESS_MIGRATION = 112, /* seamless-migration for SPICE */
> +    QEMU_CAPS_BLOCK_COMMIT       = 113, /* block-commit */
>
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 85b0bc2..68c6d3f 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2786,6 +2786,36 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
>      return ret;
>  }
>
> +/* Start a block-commit block job.  bandwidth is in MB/sec.  */
> +int
> +qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device,
> +                       const char *top, const char *base,
> +                       unsigned long bandwidth)
> +{
> +    int ret = -1;
> +    unsigned long long speed;
> +
> +    VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld",
> +              mon, device, NULLSTR(top), NULLSTR(base), bandwidth);
> +
> +    /* Convert bandwidth MiB to bytes */
> +    speed = bandwidth;
> +    if (speed > ULLONG_MAX / 1024 / 1024) {
> +        virReportError(VIR_ERR_OVERFLOW,
> +                       _("bandwidth must be less than %llu"),
> +                       ULLONG_MAX / 1024 / 1024);
> +        return -1;
> +    }
> +    speed <<= 20;
> +
> +    if (mon->json)
> +        ret = qemuMonitorJSONBlockCommit(mon, device, top, base, speed);
> +    else
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("block-commit requires JSON monitor"));
> +    return ret;
> +}
> +
>  int qemuMonitorArbitraryCommand(qemuMonitorPtr mon,
>                                  const char *cmd,
>                                  char **reply,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 54b3a99..9bdc802 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -521,6 +521,13 @@ int qemuMonitorDiskSnapshot(qemuMonitorPtr mon,
>  int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> +int qemuMonitorBlockCommit(qemuMonitorPtr mon,
> +                           const char *device,
> +                           const char *top,
> +                           const char *base,
> +                           unsigned long bandwidth)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +
>  int qemuMonitorArbitraryCommand(qemuMonitorPtr mon,
>                                  const char *cmd,
>                                  char **reply,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index bd52ce4..501b338 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -803,6 +803,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
>
>      if (STREQ(type_str, "stream"))
>          type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL;
> +    else if (STREQ(type_str, "commit"))
> +        type = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT;
>
>      switch ((virConnectDomainEventBlockJobStatus) event) {
>      case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
> @@ -3269,6 +3271,36 @@ cleanup:
>      return ret;
>  }
>
> +/* speed is in bytes/sec */
> +int
> +qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device,
> +                           const char *top, const char *base,
> +                           unsigned long speed)

up above you're using unsigned long long for speed. If
qemuMonitorJSONMakeCommand only accepts unsigned long, shouldn't you be
limiting bandwidth in the caller to ULONG_MAX / 1024 / 1024?

> +{
> +    int ret = -1;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr reply = NULL;
> +
> +    cmd = qemuMonitorJSONMakeCommand("block-commit",
> +                                     "s:device", device,
> +                                     "U:speed", speed,
> +                                     "s:top", top,
> +                                     base ? "s:base" : NULL, base,
> +                                     NULL);
> +    if (!cmd)
> +        return -1;
> +
> +    if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
> +        goto cleanup;
> +    ret = qemuMonitorJSONCheckError(cmd, reply);
> +
> +cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}
> +
> +
>  int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon,
>                                      const char *cmd_str,
>                                      char **reply_str,
> @@ -3385,6 +3417,8 @@ static int qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry,
>      }
>      if (STREQ(type, "stream"))
>          info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL;
> +    else if (STREQ(type, "commit"))
> +        info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT;
>      else
>          info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
>
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 63b84c6..71bc6aa 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -235,6 +235,13 @@ int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon,
>                                  bool reuse);
>  int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions);
>
> +int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon,
> +                               const char *device,
> +                               const char *top,
> +                               const char *base,
> +                               unsigned long bandwidth)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +
>  int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon,
>                                      const char *cmd_str,
>                                      char **reply_str,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 58cd8da..3396258 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -909,12 +909,15 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>      if (disk) {
>          path = disk->src;
>          event = virDomainEventBlockJobNewFromObj(vm, path, type, status);
> -        /* XXX If we completed a block pull, then recompute the cached
> -         * backing chain to match.  Better would be storing the chain
> -         * ourselves rather than reprobing, but this requires
> -         * modifying domain_conf and our XML to fully track the chain
> -         * across libvirtd restarts.  */
> -        if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL &&
> +        /* XXX If we completed a block pull or commit, then recompute
> +         * the cached backing chain to match.  Better would be storing
> +         * the chain ourselves rather than reprobing, but this
> +         * requires modifying domain_conf and our XML to fully track
> +         * the chain across libvirtd restarts.  For that matter, if
> +         * qemu gains support for committing the active layer, we have
> +         * to update disk->src.  */
> +        if ((type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL ||
> +             type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT) &&
>              status == VIR_DOMAIN_BLOCK_JOB_COMPLETED)
>              qemuDomainDetermineDiskChain(driver, disk, true);
>      }

ACK dependent on your answer about unsigned long vs. unsigned long long
and "speed".


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