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(a)redhat.com&gt; > > 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(a)redhat.com&gt; > --- > 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.