[libvirt] [PATCH v3 2/5] blockjob: turn on qemu capability bit for active commit

[Peter Krempa]

On 06/19/14 01:22, Eric Blake wrote:
> Use the probing functionality added in the last patch to turn on
> a capability bit when active commit is present, and gate active
> commit on that capability.
> 
> While at it, leave a few more bread-crumbs about what blockjob
> sync/async means, and enforce that sync cancel is only ever
> encountered with blockpull (at this point, RHEL 6.2 is likely
> to be the only platform to have sync cancel).

Hmm, I don't like mentioning old rhel versions in upstream given that
this version won't work properly there. Also I have to cite one of your
previous mails:

>> On 06/14/14 14:50, Eric Blake wrote:
>
> "While at it" is often an indication that a patch does too much at once.
>


> 
> Modifying qemucapabilitiestest is painful; the .replies files would
> be so much easier if they had comments correlating which command
> generated the given reply.  Maybe I'll fix that up later...
> 
> * src/qemu/qemu_capabilities.h (QEMU_CAPS_ACTIVE_COMMIT): New
> capability.
> (QEMU_CAPS_BLOCKJOB_SYNC): Enhance comment.
> * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Likewise.
> (qemuDomainBlockCopy): Likewise.
> (qemuDomainBlockCommit): Use the new bit
> * src/qemu/qemu_capabilities.c (virQEMUCaps): Name the new bit.
> (virQEMUCapsProbeQMPCommands): Set it.
> * tests/qemucapabilitiesdata/caps_1.3.1-1.replies: Update.
> * tests/qemucapabilitiesdata/caps_1.4.2-1.replies: Likewise.
> * tests/qemucapabilitiesdata/caps_1.5.3-1.replies: Likewise.
> * tests/qemucapabilitiesdata/caps_1.6.0-1.replies: Likewise.
> * tests/qemucapabilitiesdata/caps_1.6.50-1.replies: Likewise.

It would be cool if we'd tweak the capabilities test to do automatic
response numbering so we'd don't need to rewrite a whole lot of the
stuff if we add something like this.

> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c                     |  7 +++
>  src/qemu/qemu_capabilities.h                     |  3 +-
>  src/qemu/qemu_driver.c                           | 16 +++---
>  tests/qemucapabilitiesdata/caps_1.3.1-1.replies  | 62 +++++++++++++-----------
>  tests/qemucapabilitiesdata/caps_1.4.2-1.replies  | 62 +++++++++++++-----------
>  tests/qemucapabilitiesdata/caps_1.5.3-1.replies  | 62 +++++++++++++-----------
>  tests/qemucapabilitiesdata/caps_1.6.0-1.replies  | 62 +++++++++++++-----------
>  tests/qemucapabilitiesdata/caps_1.6.50-1.replies | 62 +++++++++++++-----------
>  8 files changed, 193 insertions(+), 143 deletions(-)
> 

> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index d755caa..617986d 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -127,7 +127,7 @@ enum virQEMUCapsFlags {
>      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_BLOCKJOB_ASYNC     = 91, /* qemu 1.1/RHEL 6.3 block-job-cancel */

This hunk seems unrelated. I don't think upstream cares about RHEL 6.3
now that we have 6.5 and also this version wouldn't really work there.

>      QEMU_CAPS_SCSI_CD            = 92, /* -device scsi-cd */
>      QEMU_CAPS_IDE_CD             = 93, /* -device ide-cd */
>      QEMU_CAPS_NO_USER_CONFIG     = 94, /* -no-user-config */

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ca58d6b..004b63d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15130,8 +15130,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
>       * to prevent newly scheduled block jobs from confusing us.  */
>      if (mode == BLOCK_JOB_ABORT) {
>          if (!async) {
> -            /* Older qemu that lacked async reporting also lacked
> -             * active commit, so we can hardcode the event to pull.
> +            /* Older qemu (RHEL 6.2) that lacked async reporting also
> +             * lacked copy and commit, so we can hardcode type_pull.
>               * We have to generate two variants of the event. */
>              int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL;
>              int status = VIR_DOMAIN_BLOCK_JOB_CANCELED;

Unrelated again ...


> @@ -15291,6 +15291,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
>          goto endjob;
>      }
> 
> +    /* Ensure that no one backports copy to RHEL 6.2, where cancel
> +     * behaved differently */
>      if (!(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_MIRROR) &&
>            virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC))) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

Capability adding parts look okay, but I'm not okay with mentioning the
obsolete RHEL stuff in upstream. And if somebody else thinks it's okay,
it at least shouldn't be part of this patch.

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140619/25a71b26/attachment-0001.sig>