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 <[email protected]> > --- > 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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
