Re: [PATCH 1/2] python/machine.py: upgrade vm.command() method

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

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.

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.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|