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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D435CC433EF for ; Thu, 30 Sep 2021 17:23:07 +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 27D686137A for ; Thu, 30 Sep 2021 17:23:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 27D686137A 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]:60334 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mVzly-00035X-4F for qemu-devel@archiver.kernel.org; Thu, 30 Sep 2021 13:23:06 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:37752) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mVzkE-0001mX-Vz for qemu-devel@nongnu.org; Thu, 30 Sep 2021 13:21:19 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:49008) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mVzkA-0002Cb-4V for qemu-devel@nongnu.org; Thu, 30 Sep 2021 13:21:16 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1633022472; 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=biaV+fDSeKoR3XFlMjo3WvN53073Uls9ThF0qIzDSAs=; b=GxUp9vn39u8EeyXrn2ot0RswlcbH2w9tyeVeGVi6tzVaab+fRE8efuoWk3Dlg7dIF4Qmnc hErRe56ehC7SuCPa4+lvUAFh0wdiC6H8SQtthxZ3Ty3/dHXkhR70LXfKq+SEbX5K+qIm3N +2jxHUA62rekKpaJnMpi/uMLRiAJ2M8= Received: from mail-vk1-f197.google.com (mail-vk1-f197.google.com [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-323-q0nBtsCbNcqKepxHgaeKtQ-1; Thu, 30 Sep 2021 13:21:10 -0400 X-MC-Unique: q0nBtsCbNcqKepxHgaeKtQ-1 Received: by mail-vk1-f197.google.com with SMTP id bi54-20020a05612218b600b0029578be32dfso2101004vkb.18 for ; Thu, 30 Sep 2021 10:21:10 -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=biaV+fDSeKoR3XFlMjo3WvN53073Uls9ThF0qIzDSAs=; b=QFNzkCKVtj9i8VxWHtWi8osAjBhSA5aoP8gfOZgto9/+GkQ2qKWmXZgEC5Txp2hQDT dGxUCjO16uaOU8eh4+F9nngK4Se3Blq2Ujm9j4pYhwrQWlNgvdvQEGNpxPwCBF6Zt+1y lzlUeinIOJAAIe+SbZxpMMvyMHF6A8arZbNdj1ERE45D8gHSpVy/XfnPD2jhr98QeYLv V1jNz0m1JBbniPobPEsCQMeORv61HvG1JhMHT9TtntY76MG+HwycjRIZTYMMKSuWZ65D zQAqwRJ5Dc1nJ7NMgXUxX8Z+N9mQ//ZrhkqNCagAGeYQUIgj7HbXwJehv6LUAqylNJ+Q fWLg== X-Gm-Message-State: AOAM531v1YSctLJ6Ed3SL4DMhToy9wDB0Xzi6GwkrMRzXu3oGWkpYklR DMP5Trg31FCoH6s5EiKY34pn8xCG2o4h+2VIaX46UiLZj/tMN0PkiAZQW6eI1N8Ot2u+B8HXFwQ jumV4HGmJHJ05oXNBpXNm138XG0lQXKE= X-Received: by 2002:a05:6102:3a0c:: with SMTP id b12mr500164vsu.13.1633022469782; Thu, 30 Sep 2021 10:21:09 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzxHGSVNU2K+Os/TzNLkoLQjAc+kEZJnk9Nlqtfs4ZpGtF/XIqRPkhGghWmjNn7I6eYUeMEmgmVO8aVvl868is= X-Received: by 2002:a05:6102:3a0c:: with SMTP id b12mr500130vsu.13.1633022469599; Thu, 30 Sep 2021 10:21:09 -0700 (PDT) MIME-Version: 1.0 References: <20210929194428.1038496-1-jsnow@redhat.com> <20210929194428.1038496-7-jsnow@redhat.com> <87y27eqwkz.fsf@dusky.pond.sub.org> In-Reply-To: <87y27eqwkz.fsf@dusky.pond.sub.org> From: John Snow Date: Thu, 30 Sep 2021 13:20:58 -0400 Message-ID: Subject: Re: [PATCH v3 06/13] qapi/parser: remove FIXME comment from _append_body_line To: Markus Armbruster 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="0000000000003606ef05cd39acbe" 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: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, 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: Michael Roth , Cleber Rosa , Eric Blake , qemu-devel , Eduardo Habkost Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --0000000000003606ef05cd39acbe Content-Type: text/plain; charset="UTF-8" On Thu, Sep 30, 2021 at 4:47 AM Markus Armbruster wrote: > John Snow writes: > > > True, we do not check the validity of this symbol -- but we don't check > > the validity of definition names during parse, either -- that happens > > later, during the expr check. I don't want to introduce a dependency on > > expr.py:check_name_str here and introduce a cycle. > > > > Instead, rest assured that a documentation block is required for each > > definition. This requirement uses the names of each section to ensure > > that we fulfilled this requirement. > > > > e.g., let's say that block-core.json has a comment block for > > "Snapshot!Info" by accident. We'll see this error message: > > > > In file included from ../../qapi/block.json:8: > > ../../qapi/block-core.json: In struct 'SnapshotInfo': > > ../../qapi/block-core.json:38: documentation comment is for > 'Snapshot!Info' > > > > That's a pretty decent error message. > > > > Now, let's say that we actually mangle it twice, identically: > > > > ../../qapi/block-core.json: In struct 'Snapshot!Info': > > ../../qapi/block-core.json:38: struct has an invalid name > > > > That's also pretty decent. If we forget to fix it in both places, we'll > > just be back to the first error. > > > > Therefore, let's just drop this FIXME and adjust the error message to > > not imply a more thorough check than is actually performed. > > > > Signed-off-by: John Snow > > --- > > scripts/qapi/parser.py | 6 ++++-- > > tests/qapi-schema/doc-empty-symbol.err | 2 +- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 2f93a752f66..52748e8e462 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -558,9 +558,11 @@ def _append_body_line(self, line): > > raise QAPIParseError( > > self._parser, "extra whitespace around symbol > declaration") > > self.symbol = line[1:-1] > > - # FIXME invalid names other than the empty string aren't > flagged > > + # Invalid names are not checked here, but the name provided > MUST > > + # match the following definition, which *is* validated. > > if not self.symbol: > > - raise QAPIParseError(self._parser, "invalid name") > > + raise QAPIParseError( > > + self._parser, "doc symbol name cannot be blank") > > "blank" feels like "empty or just whitespace" to me. We actually mean > "empty". > > What about "name required after @"? > > Sure, yeah. Updated. > > elif self.symbol: > > # This is a definition documentation block > > name = line.split(' ', 1)[0] > > diff --git a/tests/qapi-schema/doc-empty-symbol.err > b/tests/qapi-schema/doc-empty-symbol.err > > index 81b90e882a7..a4981ee28ea 100644 > > --- a/tests/qapi-schema/doc-empty-symbol.err > > +++ b/tests/qapi-schema/doc-empty-symbol.err > > @@ -1 +1 @@ > > -doc-empty-symbol.json:4:1: invalid name > > +doc-empty-symbol.json:4:1: doc symbol name cannot be blank > > --0000000000003606ef05cd39acbe Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Sep 30, 2021 at 4:47 AM Marku= s Armbruster <armbru@redhat.com= > wrote:
John= Snow <jsnow@redha= t.com> writes:

> True, we do not check the validity of this symbol -- but we don't = check
> the validity of definition names during parse, either -- that happens<= br> > later, during the expr check. I don't want to introduce a dependen= cy on
> expr.py:check_name_str here and introduce a cycle.
>
> Instead, rest assured that a documentation block is required for each<= br> > definition. This requirement uses the names of each section to ensure<= br> > that we fulfilled this requirement.
>
> e.g., let's say that block-core.json has a comment block for
> "Snapshot!Info" by accident. We'll see this error messag= e:
>
> In file included from ../../qapi/block.json:8:
> ../../qapi/block-core.json: In struct 'SnapshotInfo':
> ../../qapi/block-core.json:38: documentation comment is for 'Snaps= hot!Info'
>
> That's a pretty decent error message.
>
> Now, let's say that we actually mangle it twice, identically:
>
> ../../qapi/block-core.json: In struct 'Snapshot!Info':
> ../../qapi/block-core.json:38: struct has an invalid name
>
> That's also pretty decent. If we forget to fix it in both places, = we'll
> just be back to the first error.
>
> Therefore, let's just drop this FIXME and adjust the error message= to
> not imply a more thorough check than is actually performed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>=C2=A0 scripts/qapi/parser.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0| 6 ++++--
>=C2=A0 tests/qapi-schema/doc-empty-symbol.err | 2 +-
>=C2=A0 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 2f93a752f66..52748e8e462 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -558,9 +558,11 @@ def _append_body_line(self, line):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QA= PIParseError(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 self._parser, "extra whitespace around symbol declaration"= )
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.symbol =3D line[1= :-1]
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # FIXME invalid names other= than the empty string aren't flagged
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Invalid names are not che= cked here, but the name provided MUST
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # match the following defin= ition, which *is* validated.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if not self.symbol: > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPIPar= seError(self._parser, "invalid name")
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPIPar= seError(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= self._parser, "doc symbol name cannot be blank")

"blank" feels like "empty or just whitespace" to me.=C2= =A0 We actually mean
"empty".

What about "name required after @"?


Sure, yeah. Updated.
=C2= =A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 elif self.symbol:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # This is a definition= documentation block
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 name =3D line.split(&#= 39; ', 1)[0]
> diff --git a/tests/qapi-schema/doc-empty-symbol.err b/tests/qapi-schem= a/doc-empty-symbol.err
> index 81b90e882a7..a4981ee28ea 100644
> --- a/tests/qapi-schema/doc-empty-symbol.err
> +++ b/tests/qapi-schema/doc-empty-symbol.err
> @@ -1 +1 @@
> -doc-empty-symbol.json:4:1: invalid name
> +doc-empty-symbol.json:4:1: doc symbol name cannot be blank

--0000000000003606ef05cd39acbe--