Re: [PATCH 13/28] qapi: Enforce event naming rules

From: John Snow <jsnow@redhat.com>
To: Markus Armbruster <armbru@redhat.com>, qemu-devel@nongnu.org
Cc: michael.roth@amd.com, marcandre.lureau@redhat.com
Subject: Re: [PATCH 13/28] qapi: Enforce event naming rules
Date: Tue, 23 Mar 2021 18:31:47 -0400	[thread overview]
Message-ID: <bd1b8230-30fd-a4a4-d38c-8650e645c586@redhat.com> (raw)
In-Reply-To: <20210323094025.3569441-14-armbru@redhat.com>

On 3/23/21 5:40 AM, Markus Armbruster wrote:
> Event names should be ALL_CAPS with words separated by underscore.
> Enforce this.  The only offenders are in tests/.  Fix them.  Existing
> test event-case covers the new error.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/unit/test-qmp-event.c               |  6 +++---
>   scripts/qapi/expr.py                      |  4 +++-
>   tests/qapi-schema/doc-good.json           |  4 ++--
>   tests/qapi-schema/doc-good.out            |  4 ++--
>   tests/qapi-schema/doc-good.txt            |  2 +-
>   tests/qapi-schema/doc-invalid-return.json |  4 ++--
>   tests/qapi-schema/event-case.err          |  2 ++
>   tests/qapi-schema/event-case.json         |  2 --
>   tests/qapi-schema/event-case.out          | 14 --------------
>   tests/qapi-schema/qapi-schema-test.json   |  6 +++---
>   tests/qapi-schema/qapi-schema-test.out    |  8 ++++----
>   11 files changed, 22 insertions(+), 34 deletions(-)
> 
> diff --git a/tests/unit/test-qmp-event.c b/tests/unit/test-qmp-event.c
> index 047f44ff9a..d58c3b78f2 100644
> --- a/tests/unit/test-qmp-event.c
> +++ b/tests/unit/test-qmp-event.c
> @@ -143,7 +143,7 @@ static void test_event_d(TestEventData *data,
>   
>   static void test_event_deprecated(TestEventData *data, const void *unused)
>   {
> -    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES1' }");
> +    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES1' }");
>   
>       memset(&compat_policy, 0, sizeof(compat_policy));
>   
> @@ -163,7 +163,7 @@ static void test_event_deprecated_data(TestEventData *data, const void *unused)
>   {
>       memset(&compat_policy, 0, sizeof(compat_policy));
>   
> -    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0',"
> +    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0',"
>                                              " 'data': { 'foo': 42 } }");
>       qapi_event_send_test_event_features0(42);
>       g_assert(data->emitted);
> @@ -172,7 +172,7 @@ static void test_event_deprecated_data(TestEventData *data, const void *unused)
>   
>       compat_policy.has_deprecated_output = true;
>       compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE;
> -    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0' }");
> +    data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0' }");
>       qapi_event_send_test_event_features0(42);
>       g_assert(data->emitted);
>   
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index b5fb0be48b..c065505b27 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -45,7 +45,9 @@ def check_name_str(name, info, source):
>   
>   def check_name_upper(name, info, source):
>       stem = check_name_str(name, info, source)
> -    # TODO reject '[a-z-]' in @stem
> +    if re.search(r'[a-z-]', stem):
> +        raise QAPISemError(
> +            info, "name of %s must not use lowercase or '-'" % source)
>   

Does a little bit more than check_name_upper. Is this only used for 
event names? I guess so. Should it be inlined into check_defn_name_str 
instead in this case, or nah?