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 X-Spam-Level: X-Spam-Status: No, score=-10.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7BD7AC433EF for ; Fri, 17 Sep 2021 23:14:20 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id C41E561130 for ; Fri, 17 Sep 2021 23:14:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org C41E561130 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:59524 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mRN3i-0006UL-O0 for qemu-devel@archiver.kernel.org; Fri, 17 Sep 2021 19:14:18 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:60596) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mRN2d-0005gI-M5 for qemu-devel@nongnu.org; Fri, 17 Sep 2021 19:13:11 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:46467) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mRN2Z-0004R7-II for qemu-devel@nongnu.org; Fri, 17 Sep 2021 19:13:10 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1631920385; 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=CrJMLXDeAkx6N+HWLhQjvpOy4/p3y2bbW9fvyddJpIA=; b=O6DaGsF9DVb+AO1eivufsSgmrFlj7VqpUErPxE+TSTRPUjHZ9RiDEUaJKF+V9fD9w9sAjq 01H1nWWTTuLFmo7sUfF1qyIdAEJetD+S1hy/J9FuJfXVxWH3TW2BDVODqhtLkqwHrggUED YTsY+p42NRBJiMJI/u8qZWNa0iOwjLw= Received: from mail-oo1-f72.google.com (mail-oo1-f72.google.com [209.85.161.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-567-2T_HfNy9PwSdwxwiAAM_BQ-1; Fri, 17 Sep 2021 19:13:04 -0400 X-MC-Unique: 2T_HfNy9PwSdwxwiAAM_BQ-1 Received: by mail-oo1-f72.google.com with SMTP id d18-20020a4a9cd2000000b0029afc2f9586so1678430ook.19 for ; Fri, 17 Sep 2021 16:13:03 -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=CrJMLXDeAkx6N+HWLhQjvpOy4/p3y2bbW9fvyddJpIA=; b=LMS7Loi8jDSxHiopw4ldKQsVsikE53ocE72E21UizspBVdNxJmqg+Qt8bcTQiNxns3 fSEYx6cYO1utBahD6KVF4Hz1BSyB9qMhFc98JGGE9U+mWaIyfbf39KPavPM+cgZzPno4 LIboC2PC+i+S/Fjqx7/jX4hLsHcyzgv5LM0yP0eJsRrjEkjzC2fbAoCeBx3/Sm6Z/uq2 Ft8Uslfg6t+0Ps04anvKqyiVesyng2l1zv894jarzsP7yvfyffGXrQU3w+XeU4rwnvma LH8JbdCMW4CZjCkv28SVsQgZGeKjGIAjXF9Wa3jXZy12FbijWDljSrHWcMIrzcVf1tyL E2FA== X-Gm-Message-State: AOAM533rUggzOEmkZAU4KotQMPStqKFuaApX8AzqWJHOpkQE/DitvKNW FPrOIPsWkEtGzl+2IKS5g5qY67uAAw1Eqfmi+1+x/Xh3W8gob/s1T7ofbmZqev8nm7pUrPZaPdj Am8Pt+ONdHl83Smz9JszTWhTeqUJ+uXE= X-Received: by 2002:aca:3887:: with SMTP id f129mr7565119oia.112.1631920383213; Fri, 17 Sep 2021 16:13:03 -0700 (PDT) X-Google-Smtp-Source: ABdhPJx6UHrn5oxcyZfAzJrO6RxKz7TuGqbYgUllM0OM5F/6eMxn7EkcdoUsA5eR7jjGK6NxR7pnpeuzUcj3Vka3920= X-Received: by 2002:aca:3887:: with SMTP id f129mr7565102oia.112.1631920382979; Fri, 17 Sep 2021 16:13:02 -0700 (PDT) MIME-Version: 1.0 References: <20210917054047.2042843-1-jsnow@redhat.com> <20210917054047.2042843-10-jsnow@redhat.com> In-Reply-To: From: John Snow Date: Fri, 17 Sep 2021 19:12:51 -0400 Message-ID: Subject: Re: [PATCH 09/15] python/machine: remove has_quit argument To: Hanna Reitz 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="000000000000bac95b05cc3912e3" Received-SPF: pass client-ip=216.205.24.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -31 X-Spam_score: -3.2 X-Spam_bar: --- X-Spam_report: (-3.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.392, 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_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Vladimir Sementsov-Ogievskiy , Eduardo Habkost , qemu-block@nongnu.org, qemu-devel , Cleber Rosa Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000bac95b05cc3912e3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Sep 17, 2021 at 9:59 AM Hanna Reitz wrote: > On 17.09.21 07:40, John Snow wrote: > > If we spy on the QMP commands instead, we don't need callers to remembe= r > > to pass it. Seems like a fair trade-off. > > > > The one slightly weird bit is overloading this instance variable for > > wait(), where we use it to mean "don't issue the qmp 'quit' > > command". This means that wait() will "fail" if the QEMU process does > > not terminate of its own accord. > > > > In most cases, we probably did already actually issue quit -- some > > iotests do this -- but in some others, we may be waiting for QEMU to > > terminate for some other reason. > > > > Signed-off-by: John Snow > > --- > > python/qemu/machine/machine.py | 35 +++++++++++++++++++--------------= - > > tests/qemu-iotests/040 | 7 +------ > > tests/qemu-iotests/218 | 2 +- > > tests/qemu-iotests/255 | 2 +- > > 4 files changed, 23 insertions(+), 23 deletions(-) > > Looks good overall, some bikeshedding below. > > > diff --git a/python/qemu/machine/machine.py > b/python/qemu/machine/machine.py > > index 056d340e35..6e58d2f951 100644 > > --- a/python/qemu/machine/machine.py > > +++ b/python/qemu/machine/machine.py > > @@ -170,6 +170,7 @@ def __init__(self, > > self._console_socket: Optional[socket.socket] =3D None > > self._remove_files: List[str] =3D [] > > self._user_killed =3D False > > + self._has_quit =3D False > > Sounds a bit weird, because evidently it has quit. > > I mean, it was always more of a has_sent_quit or quit_issued, but now it > really seems a bit wrong. > > "quit_issued" seems like it might help overall, I'll do that. > [...] > > > @@ -529,7 +526,9 @@ def wait(self, timeout: Optional[int] =3D 30) -> No= ne: > > :param timeout: Optional timeout in seconds. Default 30 > seconds. > > A value of `None` is an infinite wait. > > """ > > - self.shutdown(has_quit=3DTrue, timeout=3Dtimeout) > > + # In case QEMU was stopped without issuing 'quit': > > This comment confused me more than anything and only looking at the > function=E2=80=99s name and doc string was I able to understand why we ha= ve > has_quit =3D True here, and only by scratching my head asking myself why > you=E2=80=99d write the comment did I understand the comment=E2=80=99s pu= rpose. > > What isn=E2=80=99t quite clear in the comment is that we kind of expect > _has_quit to already be True, because the VM was probably exited with > `quit`. But we don=E2=80=99t know for sure, so we have to force _has_qui= t to True. > > But it=E2=80=99s also not necessary to explain, I think. The idea is sim= ply > that this function should do nothing to make the VM quit, so it=E2=80=99s > absolutely natural that we=E2=80=99d set _has_quit to True, because the c= aller > guarantees that they already made the VM quit. No need to explain why > this might already be True, and why it=E2=80=99s still OK. > > So I=E2=80=99d just say something like =E2=80=9CDo not send a `quit` to t= he VM, just > wait for it to exit=E2=80=9D. > > I'll just drop the comment, then. > Hanna > > > + self._has_quit =3D True > > + self.shutdown(timeout=3Dtimeout) > > > > def set_qmp_monitor(self, enabled: bool =3D True) -> None: > > """ > > --000000000000bac95b05cc3912e3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Sep 17, 2021 at 9:59 AM Hanna= Reitz <hreitz@redhat.com> w= rote:
On 17.09.2= 1 07:40, John Snow wrote:
> If we spy on the QMP commands instead, we don't need callers to re= member
> to pass it. Seems like a fair trade-off.
>
> The one slightly weird bit is overloading this instance variable for > wait(), where we use it to mean "don't issue the qmp 'qui= t'
> command". This means that wait() will "fail" if the QEM= U process does
> not terminate of its own accord.
>
> In most cases, we probably did already actually issue quit -- some
> iotests do this -- but in some others, we may be waiting for QEMU to > terminate for some other reason.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>=C2=A0 =C2=A0python/qemu/machine/machine.py | 35 +++++++++++++++++++---= ------------
>=C2=A0 =C2=A0tests/qemu-iotests/040=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|= =C2=A0 7 +------
>=C2=A0 =C2=A0tests/qemu-iotests/218=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|= =C2=A0 2 +-
>=C2=A0 =C2=A0tests/qemu-iotests/255=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|= =C2=A0 2 +-
>=C2=A0 =C2=A04 files changed, 23 insertions(+), 23 deletions(-)

Looks good overall, some bikeshedding below.

> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/mach= ine.py
> index 056d340e35..6e58d2f951 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -170,6 +170,7 @@ def __init__(self,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self._console_socket: Optional= [socket.socket] =3D None
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self._remove_files: List[str] = =3D []
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self._user_killed =3D False > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._has_quit =3D False

Sounds a bit weird, because evidently it has quit.

I mean, it was always more of a has_sent_quit or quit_issued, but now it really seems a bit wrong.


"quit_issued" seems like it = might help overall, I'll do that.
=C2=A0
[...]

> @@ -529,7 +526,9 @@ def wait(self, timeout: Optional[int] =3D 30) ->= ; None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0:param timeout: Optional timeo= ut in seconds. Default 30 seconds.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0A value of `None` is an infinite wait.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"""
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.shutdown(has_quit=3DTrue, timeout=3D= timeout)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # In case QEMU was stopped without issuin= g 'quit':

This comment confused me more than anything and only looking at the
function=E2=80=99s name and doc string was I able to understand why we have=
has_quit =3D True here, and only by scratching my head asking myself why you=E2=80=99d write the comment did I understand the comment=E2=80=99s purp= ose.

What isn=E2=80=99t quite clear in the comment is that we kind of expect _has_quit to already be True, because the VM was probably exited with
`quit`.=C2=A0 But we don=E2=80=99t know for sure, so we have to force _has_= quit to True.

But it=E2=80=99s also not necessary to explain, I think.=C2=A0 The idea is = simply
that this function should do nothing to make the VM quit, so it=E2=80=99s <= br> absolutely natural that we=E2=80=99d set _has_quit to True, because the cal= ler
guarantees that they already made the VM quit.=C2=A0 No need to explain why=
this might already be True, and why it=E2=80=99s still OK.

So I=E2=80=99d just say something like =E2=80=9CDo not send a `quit` to the= VM, just
wait for it to exit=E2=80=9D.


I'll just drop the comment, then. =
=C2=A0