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

[Daniel P . Berrangé]

On Tue, Apr 19, 2022 at 01:08:06PM -0400, John Snow wrote:
> On Tue, Apr 19, 2022, 12:42 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > 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'd be happy with a single 'cmd' method with 'check=False' to turn
off exceptions on demand.

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 :|