From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 458F0C433EF for ; Tue, 19 Apr 2022 17:09:47 +0000 (UTC) Received: from localhost ([::1]:56472 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ngrMI-0005AN-D8 for qemu-devel@archiver.kernel.org; Tue, 19 Apr 2022 13:09:46 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34046) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ngrKw-0003br-DJ for qemu-devel@nongnu.org; Tue, 19 Apr 2022 13:08:25 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:36638) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ngrKt-0004Z3-B9 for qemu-devel@nongnu.org; Tue, 19 Apr 2022 13:08:20 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1650388098; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=RaMyzwLWJ7vVM4uzXGYnow+dYxLVwUeQz0kuDm72Mn0=; b=OmvgUdrHpT7h5TmcSKIUIDRiKq7KXVc9wNRYWweVk7e8oAlO3nBcylSDcj/Xw4rW4xU5j7 IjAl6EZ87wT6LAtym7wSIhBjyaX7QbMoSKuklST7/RDCoO/Czho3mkTc+Iz/OmzDSOVg5U qiEBsE1AucW+kKA22BJbpUYWt1hcsEg= Received: from mail-vk1-f199.google.com (mail-vk1-f199.google.com [209.85.221.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-619-keNLLjvuPHG-yv8jXR7L9g-1; Tue, 19 Apr 2022 13:08:16 -0400 X-MC-Unique: keNLLjvuPHG-yv8jXR7L9g-1 Received: by mail-vk1-f199.google.com with SMTP id t129-20020a1faa87000000b00349c9e1ceebso153787vke.17 for ; Tue, 19 Apr 2022 10:08:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RaMyzwLWJ7vVM4uzXGYnow+dYxLVwUeQz0kuDm72Mn0=; b=OYria3nFON3UBYPp/eHqSYPb9xqDg2ITxXKv78EPwpJA/tfImr3ywOjqxs8niVDmzU FPnj2bGwkk+a/0FQ7+GaIz70XMCoLOQgZeED+H9b5CXrFzF9oOae5NF3+CJt3Z24Aq7v xayFRLaveAkr8XHCIBEkUDcekHwC7z64PtxPpbRnS5NJKteez/343+Iske1tZKgLQp0v zSRKNdZtc4akW/OTwLVzj8czsU9IBoQKnRjy7Qvo+uQMkpuCDE25Vvi4pp9Ve4Bsmy5O ffclGfIaYIvjIT0d+Twyizd40HafJqXgyfOiqXUdYdV90g9VKcOFr/xjhKJ+P+oJmXfD zNrg== X-Gm-Message-State: AOAM532xecPj1e31Dnu1fWDNq5LZe5IP3Ka1K1hnSaRyS0ZSRPFcfmMf moNBELaam14LidD/7tjp+MbaW/Uupmj4+DwexgDylzbDJApu7VWZZmKth6AltIrnPRHmh/k3eOJ 4JzbsjPrSGkqdoLU+zhvbr7C6nP/+C9Q= X-Received: by 2002:ab0:67cf:0:b0:341:257f:ce52 with SMTP id w15-20020ab067cf000000b00341257fce52mr4435840uar.109.1650388096252; Tue, 19 Apr 2022 10:08:16 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyWhiHMfnZNdg5SEsui12EZjaJeAEghK0TPyaukuxY9FuU12HiLuqY0zeACpsnTFzIhIt8GzjwNvZfpwmPqG+U= X-Received: by 2002:ab0:67cf:0:b0:341:257f:ce52 with SMTP id w15-20020ab067cf000000b00341257fce52mr4435832uar.109.1650388096044; Tue, 19 Apr 2022 10:08:16 -0700 (PDT) MIME-Version: 1.0 References: <20220408170214.45585-1-vsementsov@openvz.org> <20220408170214.45585-2-vsementsov@openvz.org> In-Reply-To: From: John Snow Date: Tue, 19 Apr 2022 13:08:06 -0400 Message-ID: Subject: Re: [PATCH 1/2] python/machine.py: upgrade vm.command() method To: =?UTF-8?Q?Daniel_P=2E_Berrang=C3=A9?= Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=jsnow@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="00000000000034e31805dd04ec75" Received-SPF: pass client-ip=170.10.129.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Beraldo Leal , Qemu-block , Vladimir Sementsov-Ogievskiy , qemu-devel , Hanna Reitz , vsementsov@openvz.org, Cleber Rosa Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000034e31805dd04ec75 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Apr 19, 2022, 12:42 PM Daniel P. Berrang=C3=A9 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 > > --- > > 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 =3D True > > return ret > > > > - def command(self, cmd: str, > > - conv_keys: bool =3D 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=3DFalse 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 > --00000000000034e31805dd04ec75 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Apr 19, 2022, 12:42 PM Daniel P. Berrang=C3=A9= <berrange@redhat.com> wro= te:
On Fri, Apr 08, 2022 at 08:02:1= 3PM +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<= br> > vm.command() support same interface improvements like in vm.qmp() and<= br> > rename to shorter vm.cmd().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.o= rg>
> ---
>=C2=A0 python/qemu/machine/machine.py | 16 ++++++++++++---
>=C2=A0 tests/qemu-iotests/256=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 34 +++= +++++++++++++----------------
>=C2=A0 tests/qemu-iotests/257=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 36 +++= ++++++++++++++-----------------
>=C2=A0 3 files changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/mach= ine.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,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._quit_issued =3D = True
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret
>=C2=A0
> -=C2=A0 =C2=A0 def command(self, cmd: str,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 conv_keys: bo= ol =3D True,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 **args: Any) = -> QMPReturnValue:
> +=C2=A0 =C2=A0 def cmd(self, cmd: str,

FYI, the original 'command' name matches semantics of 'command&= #39;
in the QEMUMonitorProtocol class.=C2=A0 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 QEMUMonitorP= rotocol
is horrible naming though. How is anyone supposed to remember which
name raises the exception and which doesn't ? Urgh.=C2=A0

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:

=C2=A0- 'cmd' runs without error checking, just returning the
=C2=A0 =C2=A0reply data as is

=C2=A0- '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 singl= e method "execute()", mimicking the name of the field used over t= he wire. It uses semantics like command() here, in that it raises an except= ion on error and returns only the response data and not the entire QMP resp= onse.

I'm in favor o= f, broadly, an approach wherein the default behavior raises an exception an= d the caller must opt-in to squelch it; either via try-except or a check=3D= False argument. It should be a conscious decision.
<= br>
(I realize this is not what the majority of iote= sts already does, but I consider that a problem to fix and not a feature. S= ee also my recent efforts to make qemu_img() and qemu_io() raise a CalledPr= ocessError 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 interfa= ce and onto the native async qmp. I believe using asyncio subprocess and as= yncio qmp will give better failure characteristics and better error readout= s.

I've prototyped t= his 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 p= redisposition to not want to steer far away from what the async api provide= s. Just some roadmap info in case we need to collaborate better on the futu= re of this module.

--js<= /div>
--00000000000034e31805dd04ec75--