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@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]).
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list