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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list