[libvirt] [PATCH 04/20] Remove upfront check for hmp - just try it cope with failure

Eric Blake eblake at redhat.com
Tue Sep 11 23:59:06 UTC 2012


On 09/11/2012 08:11 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> 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 at redhat.com>
> ---
>  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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120911/38fa94d4/attachment-0001.sig>


More information about the libvir-list mailing list