Re: [PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions

From: John Snow <jsnow@redhat.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>,
	Michael Roth <mdroth@linux.vnet.ibm.com>,
	Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions
Date: Thu, 24 Sep 2020 20:58:20 -0400	[thread overview]
Message-ID: <4ab923a8-ff03-474a-4702-2534e3744b5e@redhat.com> (raw)
In-Reply-To: <20200923202509.GK3717385@habkost.net>

On 9/23/20 4:25 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:13:12PM -0400, John Snow wrote:
>> There's not a big obvious difference between the types of checks that
>> happen in the main function versus the kind that happen in the
>> functions. Now they're in one place for each of the main types.
>>
>> As part of the move, spell out the required and optional keywords so
>> they're obvious at a glance.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 55 ++++++++++++++++++++++++++------------------
>>   1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 69ee9e3f18..74b2681ef8 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -333,6 +333,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'enum',
>> +               required=('enum', 'data'),
>> +               optional=('if', 'features', 'prefix'))
>> +
>>       name = expr['enum']
>>       members = expr['data']
>>       prefix = expr.get('prefix')
>> @@ -363,6 +367,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'struct',
>> +               required=('struct', 'data'),
>> +               optional=('base', 'if', 'features'))
>> +    normalize_members(expr['data'])
>> +
>>       name = cast(str, expr['struct'])  # Asserted in check_exprs
>>       members = expr['data']
>>   
>> @@ -377,6 +386,13 @@ def check_union(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'union',
>> +               required=('union', 'data'),
>> +               optional=('base', 'discriminator', 'if', 'features'))
>> +
>> +    normalize_members(expr.get('base'))
>> +    normalize_members(expr['data'])
>> +
>>       name = cast(str, expr['union'])  # Asserted in check_exprs
>>       base = expr.get('base')
>>       discriminator = expr.get('discriminator')
>> @@ -409,6 +425,11 @@ def check_alternate(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: Expression to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'alternate',
>> +               required=('alternate', 'data'),
>> +               optional=('if', 'features'))
>> +    normalize_members(expr['data'])
>> +
>>       members = expr['data']
>>   
>>       if not members:
>> @@ -432,6 +453,13 @@ def check_command(expr: Expression, info: QAPISourceInfo) -> None:
>>       :param expr: `Expression` to validate.
>>       :param info: QAPI source file information.
>>       """
>> +    check_keys(expr, info, 'command',
>> +               required=['command'],
>> +               optional=('data', 'returns', 'boxed', 'if', 'features',
>> +                         'gen', 'success-response', 'allow-oob',
>> +                         'allow-preconfig'))
>> +    normalize_members(expr.get('data'))
>> +
>>       args = expr.get('data')
>>       rets = expr.get('returns')
>>       boxed = expr.get('boxed', False)
>> @@ -453,6 +481,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None:
>>           if:       `Optional[Ifcond]`
>>           features: `Optional[Features]`
>>       """
>> +    normalize_members(expr.get('data'))
>> +    check_keys(expr, info, 'event',
>> +               required=['event'],
>> +               optional=('data', 'boxed', 'if', 'features'))
> 
> Why is the order reversed here?  The other functions call
> check_keys() before normalize_members().
> 

Oops! This was from an earlier experiment. I am dabbling with factoring 
out all of the normalization into a step that occurs discretely before 
data shape validation.

I have deep, ulterior reasons for doing this.

...But they shouldn't have leaked into this patch series, so I'll fix 
that. Thanks!

--js