On Tue, Apr 19, 2022 at 01:08:06PM -0400, John Snow wrote: > On Tue, Apr 19, 2022, 12:42 PM Daniel P. Berrangé <berra...@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 <vsement...@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 :|