[libvirt] [PATCH v3 1/5] blockjob: allow omitted arguments to QMP block-commit

[Peter Krempa]

On 06/19/14 01:22, Eric Blake wrote:
> We are about to turn on support for active block commit.  Although
> qemu 2.0 was the first version to mostly support it, that version
> mis-handles 0-length files, and doesn't have anything available for
> easy probing.  But qemu 2.1 fixed bugs, and made life simpler by
> letting the 'top' argument be optional.  Unless someone begs for
> active commit with qemu 2.0, for now we are just going to enable
> it only by probing for qemu 2.1 behavior (anyone backporting active
> commit can also backport the optional argument behavior).
> 
> Although all our actual uses of block-commit supply arguments for
> both base and top, we can omit both arguments and use a bogus
> device string to trigger an interesting behavior in qemu.  All QMP
> commands first do argument validation, failing with GenericError
> if a mandatory argument is missing.  Once that passes, the code
> in the specific command gets to do further checking, and the qemu
> developers made sure that if device is the only supplied argument,
> then the block-commit code will look up the device first, with a
> failure of DeviceNotFound, before attempting any further argument
> validation (most other validations fail with GenericError).  Thus,
> the category of error class can reliably be used to decipher
> whether the top argument was optional, which in turn implies a
> working active commit.  Since we expect our bogus device string to
> trigger an error either way, the code is written to return a
> distinct return value without spamming the logs.
> 
> * src/qemu/qemu_monitor.h (qemuMonitorBlockCommit): Allow NULL for
> top and base, for probing purposes.
> * src/qemu/qemu_monitor.c (qemuMonitorBlockCommit): Likewise.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockCommit):
> Likewise.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockCommit):
> Likewise, with special return values based on probe.
> * tests/qemumonitorjsontest.c (mymain): Enable...
> (testQemuMonitorJSONqemuMonitorJSONBlockCommit2): ...a new test.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_monitor.c      |  2 +-
>  src/qemu/qemu_monitor.h      |  3 +--
>  src/qemu/qemu_monitor_json.c | 20 ++++++++++++++++--
>  src/qemu/qemu_monitor_json.h |  3 +--
>  tests/qemumonitorjsontest.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 70 insertions(+), 7 deletions(-)
> 

...

> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index bedd959..0e4262d 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3467,14 +3467,30 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device,
>      cmd = qemuMonitorJSONMakeCommand("block-commit",
>                                       "s:device", device,
>                                       "U:speed", speed,
> -                                     "s:top", top,
> -                                     "s:base", base,
> +                                     "S:top", top,
> +                                     "S:base", base,
>                                       NULL);
>      if (!cmd)
>          return -1;
> 
>      if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
>          goto cleanup;
> +    if (!top && !base) {
> +        /* Normally we always specify top and base; but omitting them
> +         * allows for probing whether qemu is new enough to support
> +         * live commit.  */
> +        if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) {
> +            VIR_DEBUG("block-commit supports active commit");
> +            ret = -2;
> +        } else {
> +            /* This is a false negative for qemu 2.0; but probably not
> +             * worth the additional complexity to worry about it */
> +            VIR_DEBUG("block-commit requires 'top' parameter, "
> +                      "assuming it lacks active commit");
> +            ret = -3;

I think those return values should be documented in the comment for
qemuMonitorBlockCommit so that we avoid possible confusion.

> +        }
> +        goto cleanup;
> +    }
>      ret = qemuMonitorJSONCheckError(cmd, reply);
> 
>   cleanup:

> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 2099dc8..faa968f 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -1992,6 +1992,54 @@ testQemuMonitorJSONqemuMonitorJSONSendKeyHoldtime(const void *data)
>  }
> 
>  static int
> +testQemuMonitorJSONqemuMonitorJSONBlockCommit2(const void *data)
> +{
> +    virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
> +    qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
> +    int ret = -1;
> +    const char *error1 =
> +        "{"
> +        "  \"error\": {"
> +        "    \"class\": \"DeviceNotFound\","
> +        "    \"desc\": \"Device 'bogus' not found\""
> +        "  }"
> +        "}";
> +    const char *error2 =
> +        "{"
> +        "  \"error\": {"
> +        "    \"class\": \"GenericError\","
> +        "    \"desc\": \"Parameter 'top' is missing\""
> +        "  }"
> +        "}";
> +
> +    if (!test)
> +        return -1;
> +
> +    if (qemuMonitorTestAddItemParams(test, "block-commit", error1,
> +                                     "device", "\"bogus\"",
> +                                     NULL, NULL) < 0)
> +        goto cleanup;
> +
> +    if (qemuMonitorBlockCommit(qemuMonitorTestGetMonitor(test),
> +                               "bogus", NULL, NULL, 0) != -2)
> +        goto cleanup;
> +
> +    if (qemuMonitorTestAddItemParams(test, "block-commit", error2,
> +                                     "device", "\"bogus\"",
> +                                     NULL, NULL) < 0)
> +        goto cleanup;
> +
> +    if (qemuMonitorBlockCommit(qemuMonitorTestGetMonitor(test),
> +                               "bogus", NULL, NULL, 0) != -3)
> +        goto cleanup;

If these ever fail it will be hard to diagnose but not really worth
doing anything about it.

> +
> +    ret = 0;
> + cleanup:
> +    qemuMonitorTestFree(test);
> +    return ret;
> +}
> +
> +static int
>  testQemuMonitorJSONqemuMonitorJSONGetDumpGuestMemoryCapability(const void *data)
>  {
>      virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;

ACK when you document the possible return values.

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/bebf45da/attachment-0001.sig>