On 09/11/2012 08:11 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <[email protected]> > > 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 <[email protected]> > --- > 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 [email protected] +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
