On Fri, Apr 08, 2022 at 08:02:13PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> The method is not popular, we prefer use vm.qmp() and then check
> success by hand.. But that's not optimal. To simplify movement to
> vm.command() support same interface improvements like in vm.qmp() and
> rename to shorter vm.cmd().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
> python/qemu/machine/machine.py | 16 ++++++++++++---
> tests/qemu-iotests/256 | 34 ++++++++++++++++----------------
> tests/qemu-iotests/257 | 36 +++++++++++++++++-----------------
> 3 files changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 07ac5a710b..a3fb840b93 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -648,14 +648,24 @@ def qmp(self, cmd: str,
> self._quit_issued = True
> return ret
>
> - def command(self, cmd: str,
> - conv_keys: bool = True,
> - **args: Any) -> QMPReturnValue:
> + def cmd(self, cmd: str,
FYI, the original 'command' name matches semantics of 'command'
in the QEMUMonitorProtocol class. The QEMUMonitorProtocol
class has a 'cmd' method that matches semantics of 'qmp' in
QEMUMachine.
I think it is desirable to have consistency between naming in
the two classes.
Broadly agree.
The current use of both 'cmd' and 'command' in QEMUMonitorProtocol
is horrible naming though. How is anyone supposed to remember which
name raises the exception and which doesn't ? Urgh.
Also agree.
I agree with your desire to simplify things, but if anything I would
suggest we change both QEMUMonitorProtocol and QEMUMachine to have a
convention more like python's subprocess module:
- 'cmd' runs without error checking, just returning the
reply data as is
- 'check_cmd' calls 'cmd' but raises an exception on error.
Not sure I'm on board here, though.
For what it's worth, in async qmp I added a single method "execute()", mimicking the name of the field used over the wire. It uses semantics like command() here, in that it raises an exception on error and returns only the response data and not the entire QMP response.
I'm in favor of, broadly, an approach wherein the default behavior raises an exception and the caller must opt-in to squelch it; either via try-except or a check=False argument. It should be a conscious decision.
(I realize this is not what the majority of iotests already does, but I consider that a problem to fix and not a feature. See also my recent efforts to make qemu_img() and qemu_io() raise a CalledProcessError by default to improve failed test diagnostics and remove logical errors from several iotests.)
Over time, I want to migrate machine.py off of the legacy.py interface and onto the native async qmp. I believe using asyncio subprocess and asyncio qmp will give better failure characteristics and better error readouts.
I've prototyped this a little, but it's a big project and there are still pre-requisites to hit before I worry about it too much. Maybe this summer I'll tackle it. Anyway, that's nobody else's problem right now, but I have a predisposition to not want to steer far away from what the async api provides. Just some roadmap info in case we need to collaborate better on the future of this module.
--js