On 09/11/2012 08:11 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Don't bother checking for the existance of the HMP passthrough
> command. Just try to execute it, and propagate the failure.
And these days, there's very few remaining HMP passthrough commands to
worry about (meanwhile, there's some libvirt patches to write to pick up
commands that no longer require HMP passthrough, such as send-key).
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> src/qemu/qemu_monitor.c | 20 +---------------
> src/qemu/qemu_monitor_json.c | 56 +++++++++++++++-----------------------------
> src/qemu/qemu_monitor_json.h | 3 +--
> 3 files changed, 21 insertions(+), 58 deletions(-)
Much simpler. However,
> int
> -qemuMonitorCheckHMP(qemuMonitorPtr mon, const char *cmd)
> -{
> - if (!mon->json || mon->json_hmp)
> - return 1;
> -
> - if (cmd) {
> - VIR_DEBUG("HMP passthrough not supported by qemu process;"
> - " not trying HMP for command %s", cmd);
The old code used VIR_DEBUG,
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -909,6 +909,13 @@ qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon,
> if (!cmd || qemuMonitorJSONCommandWithFd(mon, cmd, scm_fd, &reply) < 0)
> goto cleanup;
>
> + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Human monitor command is not available to run %s"),
> + cmd_str);
and the new code turns it into a hard error. I think that's a correct
conversion, but the wrong choice of error code (see below [1]).
> @@ -3112,14 +3114,8 @@ int qemuMonitorJSONDriveDel(qemuMonitorPtr mon,
> goto cleanup;
>
> if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
> - if (qemuMonitorCheckHMP(mon, "drive_del")) {
> - VIR_DEBUG("drive_del command not found, trying HMP");
> - ret = qemuMonitorTextDriveDel(mon, drivestr);
> - } else {
> - VIR_ERROR(_("deleting disk is not supported. "
> - "This may leak data if disk is reassigned"));
> - ret = 1;
> - }
> + VIR_DEBUG("drive_del command not found, trying HMP");
> + ret = qemuMonitorTextDriveDel(mon, drivestr);
Another subtle case of semantic changes. The old code did a fallback
(by setting ret = 1), the new code now flat-out fails, and skips
attempting the fallback. This time, I'm not so sure the change in
semantics is correct.
> @@ -3341,12 +3333,6 @@ int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon,
> int ret = -1;
>
> if (hmp) {
> - if (!qemuMonitorCheckHMP(mon, NULL)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("HMP passthrough is not supported by qemu"
> - " process; only QMP commands can be used"));
> - return -1;
> - }
[1] here, we are going from a nice VIR_ERR_CONFIG_UNSUPPORTED to a
not-so-nice VIR_ERR_INTERNAL_ERROR.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list