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

[Daniel P. Berrange]

On Tue, Sep 11, 2012 at 05:59:06PM -0600, Eric Blake wrote:
> 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]).

There's no actual change in semantics in general since although
qemuMonitorCheckHMP() would VIR_DEBUG, the callers would then
raise an actual error.

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

This is a genuine bug - I'll fix this. We can make qemuMonitorJSONHumanCommandWithFd
return -2 on CommandNotFound, and propagate this all the way back to the callers.

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

Arguably that error code is wrong already - CONFIG_UNSUPPORTED is for
things you specify in the XML which are not supported. It isn't for
API calls really.

I should change it to OPERATION_UNSUPPORTED instad of INTERNAL_ERROR
though.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|