[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]