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

Re: [libvirt] [PATCH 1/5] blockjob: add qemu capabilities related to block pull jobs



On Wed, Apr 11, 2012 at 05:40:34PM -0600, Eric Blake wrote:
> RHEL 6.2 was released with an early version of block jobs, which only
> worked on the qed file format, where the commands were spelled with
> underscore (contrary to QMP style), and where 'block_job_cancel' was
> synchronous and did not trigger an event.
> 
> qemu 1.1 has fixed these short-comings [1][2]: the commands now work on
> multiple file types, are spelled with dash, and 'block-job-cancel' is
> asynchronous and emits an event upon conclusion.
> 
> [1]qemu commit 370521a1d6f5537ea7271c119f3fbb7b0fa57063
> [2]https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01248.html
> 
> This patch recognizes the new spellings, and fixes virDomainBlockRebase
> to give a graceful error when talking to a too-old qemu on a partial
> rebase attempt.
> 
> * src/qemu/qemu_capabilities.h (QEMU_CAPS_BLOCKJOB_SYNC)
> (QEMU_CAPS_BLOCKJOB_ASYNC): New bits.
> * src/qemu/qemu_capabilities.c (qemuCaps): Name them.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set
> them.
> (qemuMonitorJSONBlockJob): Manage both command names.
> (qemuMonitorJSONDiskSnapshot): Minor formatting fix.
> * src/qemu/qemu_monitor.h (qemuMonitorBlockJob): Alter signature.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockJob): Likewise.
> * src/qemu/qemu_monitor.c (qemuMonitorBlockJob): Pass through
> capability bit.
> * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Update callers.
> ---
>  src/qemu/qemu_capabilities.c |    3 ++
>  src/qemu/qemu_capabilities.h |    2 +
>  src/qemu/qemu_driver.c       |   22 ++++++++++++++----
>  src/qemu/qemu_monitor.c      |   11 +++++---
>  src/qemu/qemu_monitor.h      |    5 ++-
>  src/qemu/qemu_monitor_json.c |   51 +++++++++++++++++++++++-------------------
>  src/qemu/qemu_monitor_json.h |    4 ++-
>  7 files changed, 63 insertions(+), 35 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 0e09d6d..fd4ca4d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -156,6 +156,9 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>                "scsi-disk.channel",
>                "scsi-block",
>                "transaction",
> +
> +              "block-job-sync", /* 90 */
> +              "block-job-async",
>      );
> 
>  struct qemu_feature_flags {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 78cdbe0..6550d59 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -124,6 +124,8 @@ enum qemuCapsFlags {
>      QEMU_CAPS_SCSI_DISK_CHANNEL  = 87, /* Is scsi-disk.channel available? */
>      QEMU_CAPS_SCSI_BLOCK         = 88, /* -device scsi-block */
>      QEMU_CAPS_TRANSACTION        = 89, /* transaction monitor command */
> +    QEMU_CAPS_BLOCKJOB_SYNC      = 90, /* RHEL 6.2 block_job_cancel */
> +    QEMU_CAPS_BLOCKJOB_ASYNC     = 91, /* qemu 1.1 block-job-cancel */
> 
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e18e72d..f939a31 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11607,6 +11607,7 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      char *device = NULL;
>      int ret = -1;
> +    bool async = false;
> 
>      qemuDriverLock(driver);
>      virUUIDFormat(dom->uuid, uuidstr);
> @@ -11616,6 +11617,19 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
>                          _("no domain with matching uuid '%s'"), uuidstr);
>          goto cleanup;
>      }
> +    priv = vm->privateData;
> +    if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) {
> +        async = true;
> +    } else if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) {
> +        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                        _("block jobs not supported with this QEMU binary"));
> +        goto cleanup;
> +    } else if (base) {
> +        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                        _("partial block pull not supported with this "
> +                          "QEMU binary"));
> +        goto cleanup;
> +    }
> 
>      device = qemuDiskPathToAlias(vm, path, NULL);
>      if (!device) {
> @@ -11632,13 +11646,11 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
>      }
> 
>      qemuDomainObjEnterMonitorWithDriver(driver, vm);
> -    priv = vm->privateData;
> -    /* XXX - add a qemu capability check, since only qemu 1.1 or newer
> -     * supports the base argument.
> -     * XXX - libvirt should really be tracking the backing file chain
> +    /* XXX - libvirt should really be tracking the backing file chain
>       * itself, and validating that base is on the chain, rather than
>       * relying on qemu to do this.  */
> -    ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode);
> +    ret = qemuMonitorBlockJob(priv->mon, device, base, bandwidth, info, mode,
> +                              async);
>      qemuDomainObjExitMonitorWithDriver(driver, vm);
> 
>  endjob:
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index e1a8d4c..36f3832 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2773,15 +2773,18 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
>                          const char *base,
>                          unsigned long bandwidth,
>                          virDomainBlockJobInfoPtr info,
> -                        int mode)
> +                        int mode,
> +                        bool async)
>  {
>      int ret = -1;
> 
> -    VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o",
> -              mon, device, NULLSTR(base), bandwidth, info, mode);
> +    VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%lu, info=%p, mode=%o, "
> +              "async=%d", mon, device, NULLSTR(base), bandwidth, info, mode,
> +              async);
> 
>      if (mon->json)
> -        ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode);
> +        ret = qemuMonitorJSONBlockJob(mon, device, base, bandwidth, info, mode,
> +                                      async);
>      else
>          qemuReportError(VIR_ERR_INVALID_ARG, "%s",
>                          _("block jobs require JSON monitor"));
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index b480966..dc02964 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -538,8 +538,9 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
>                          const char *back,
>                          unsigned long bandwidth,
>                          virDomainBlockJobInfoPtr info,
> -                        int mode)
> -    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5);
> +                        int mode,
> +                        bool async)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

  Hum, gcc wasn't complaining on an "ATTRIBUTE_NONNULL(5)" for something
which wasn't a pointer ?

>  int qemuMonitorOpenGraphics(qemuMonitorPtr mon,
>                              const char *protocol,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index d09d779..d4d2c3e 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -939,12 +939,14 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon,
> 
>          if (STREQ(name, "human-monitor-command"))
>              *json_hmp = 1;
> -
> -        if (STREQ(name, "system_wakeup"))
> +        else if (STREQ(name, "system_wakeup"))
>              qemuCapsSet(qemuCaps, QEMU_CAPS_WAKEUP);
> -
> -        if (STREQ(name, "transaction"))
> +        else if (STREQ(name, "transaction"))
>              qemuCapsSet(qemuCaps, QEMU_CAPS_TRANSACTION);
> +        else if (STREQ(name, "block_job_cancel"))
> +            qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC);
> +        else if (STREQ(name, "block-job-cancel"))
> +            qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC);
>      }

  Hum ... seems to me that we always set bits QEMU_CAPS_BLOCKJOB_SYNC
and QEMU_CAPS_BLOCKJOB_ASYNC together, so do you envision cases where
one was set and not the other ? If not why not merge them for the sake
of one less bit to manage ?

>      ret = 0;
> @@ -3114,7 +3116,7 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
>                              const char *device, const char *file,
>                              const char *format, bool reuse)
>  {
> -    int ret;
> +    int ret = -1;
>      virJSONValuePtr cmd;
>      virJSONValuePtr reply = NULL;
> 
> @@ -3132,14 +3134,13 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
>      if (actions) {
>          if (virJSONValueArrayAppend(actions, cmd) < 0) {
>              virReportOOMError();
> -            ret = -1;
>          } else {
>              ret = 0;
>              cmd = NULL;
>          }
>      } else {
>          if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
> -        goto cleanup;
> +            goto cleanup;
> 
>          if (qemuMonitorJSONHasError(reply, "CommandNotFound") &&
>              qemuMonitorCheckHMP(mon, "snapshot_blkdev")) {
> @@ -3388,39 +3389,43 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
>                          const char *base,
>                          unsigned long bandwidth,
>                          virDomainBlockJobInfoPtr info,
> -                        int mode)
> +                        int mode,
> +                        bool async)
>  {
>      int ret = -1;
>      virJSONValuePtr cmd = NULL;
>      virJSONValuePtr reply = NULL;
>      const char *cmd_name = NULL;
> 
> -    if (base && mode != BLOCK_JOB_PULL) {
> +    if (base && (mode != BLOCK_JOB_PULL || !async)) {
>          qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                        _("only block pull supports base: %s"), base);
> +                        _("only modern block pull supports base: %s"), base);
>          return -1;
>      }
> 
> -    if (mode == BLOCK_JOB_ABORT) {
> -        cmd_name = "block_job_cancel";
> +    switch ((BLOCK_JOB_CMD) mode) {
> +    case BLOCK_JOB_ABORT:
> +        cmd_name = async ? "block-job-cancel" : "block_job_cancel";
>          cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL);
> -    } else if (mode == BLOCK_JOB_INFO) {
> +        break;
> +    case BLOCK_JOB_INFO:
>          cmd_name = "query-block-jobs";
>          cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL);
> -    } else if (mode == BLOCK_JOB_SPEED) {
> -        cmd_name = "block_job_set_speed";
> +        break;
> +    case BLOCK_JOB_SPEED:
> +        cmd_name = async ? "block-job-set-speed" : "block_job_set_speed";
>          cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
>                                           device, "U:value",
>                                           bandwidth * 1024ULL * 1024ULL,
>                                           NULL);
> -    } else if (mode == BLOCK_JOB_PULL) {
> -        cmd_name = "block_stream";
> -        if (base)
> -            cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
> -                                             device, "s:base", base, NULL);
> -        else
> -            cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device",
> -                                             device, NULL);
> +        break;
> +    case BLOCK_JOB_PULL:
> +        cmd_name = async ? "block-stream" : "block_stream";
> +        cmd = qemuMonitorJSONMakeCommand(cmd_name,
> +                                         "s:device", device,
> +                                         base ? "s:base" : NULL, base,
> +                                         NULL);
> +        break;
>      }
> 
>      if (!cmd)
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index a0f67aa..aacbb5f 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -253,7 +253,9 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
>                              const char *base,
>                              unsigned long bandwidth,
>                              virDomainBlockJobInfoPtr info,
> -                            int mode);
> +                            int mode,
> +                            bool async)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> 
>  int qemuMonitorJSONSetLink(qemuMonitorPtr mon,
>                             const char *name,

  Minor point about the extra bit, to me that's fine, ACK

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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