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@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