Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
John Snow writes: > On 3/25/21 2:18 AM, Markus Armbruster wrote: [...] >> Apropos qapi-gen testing scripts. I have scripts to show me how the >> generated code changes along the way in a branch. They evolved over a >> long time, and try to cope with changes in the tree that are hardly >> relevant anymore. By now, they could quite possibly make Frankenstein >> recoil in horror. >> > > Are they in the tree? No, because in their current state, they are incomprehensible *and* need frequent tinkering. > Largely if the generated code changes it's > invisible to me, but I rely heavily on the unit tests. I guess maybe if > they are not in a state to upstream it might not be worth the hassle to > clean them, but I don't know. The negative unit tests are fairly comprehensive, and guard against screwing up error path reasonably well. The positive unit tests compare the frontend state dumped by test-qapi.py, and compile-test the generated code. Reasonable protection against frontend screwups. Some protection against backend screwups. Plenty of unwanted code generation changes can go undetected. A tool to show generated code changes for review is useful, and having such a tool in the tree would be nice. >> As a secondary purpose, the scripts show me how output of pycodestyle-3 >> and pylint change. This would be uninteresting if the code in master >> was clean against a useful configuration of these tools. Your work has >> been making it less interesting. >> > > --js
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
On 3/25/21 2:18 AM, Markus Armbruster wrote:
John Snow writes:
On 3/24/21 1:57 AM, Markus Armbruster wrote:
John Snow writes:
On 3/23/21 5:40 AM, Markus Armbruster wrote:
Naming rules differ for the various kinds of names. To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel(). For now, these merely
wrap around check_name_str(), but that will change shortly. Replace
the other uses of check_name_str() by appropriate uses of the
wrappers. No change in behavior just yet.
Signed-off-by: Markus Armbruster
---
scripts/qapi/expr.py | 51 +++-
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index e00467636c..30285fe334 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -21,11 +21,12 @@
from .error import QAPISemError
-# Names must be letters, numbers, -, and _. They must start
with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-'[a-zA-Z][a-zA-Z0-9_-]*$')
+# Names consist of letters, digits, -, and _, starting with a letter.
+# An experimental name is prefixed with x-. A name of a downstream
+# extension is prefixed with __RFQDN_. The latter prefix goes first.
+valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
+r'(x-)?'
+r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
def check_name_is_str(name, info, source):
@@ -37,16 +38,38 @@ def check_name_str(name, info, source,
permit_upper=False):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(name) or \
- c_name(name, False).startswith('q_'):
+match = valid_name.match(name)
+if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name" % source)
+return match.group(3)
+
+
+def check_name_upper(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[a-z-]' in @stem
+
Creates (presumably) temporary errors in flake8 for the dead
assignment here and below.
All gone by the end of the series.
"make check" and checkpatch were content. Anything else you'd like me
to run?
Eventually it'll be part of CI, with targets to run locally.
I never expected the process to take this long, so I did not invest my
time in developing an interim solution.
I use a hastily written script to do my own testing, which I run for
every commit that touches QAPI:
#!/usr/bin/env bash
set -e
if [[ -f qapi/.flake8 ]]; then
echo "flake8 --config=qapi/.flake8 qapi/"
flake8 --config=qapi/.flake8 qapi/
fi
if [[ -f qapi/pylintrc ]]; then
echo "pylint --rcfile=qapi/pylintrc qapi/"
pylint --rcfile=qapi/pylintrc qapi/
fi
if [[ -f qapi/mypy.ini ]]; then
echo "mypy --config-file=qapi/mypy.ini qapi/"
mypy --config-file=qapi/mypy.ini qapi/
fi
if [[ -f qapi/.isort.cfg ]]; then
pushd qapi
echo "isort -c ."
isort -c .
popd
fi
pushd ../bin/git
make -j9
make check-qapi-schema
popd
Thanks for sharing this!
My intent, eventually, was to get scripts/qapi under python/qemu/qapi;
under which there is a testing framework thing I have been debating on
and off with Cleber for a while that does (basically) the same thing as
what my hasty script does, but more integrated with Python ecosystem.
It'll do a few things:
(1) Gets these checks on CI
(2) Allows developers to manually run the checks locally
(3) Allows developers to manually run the checks locally using a
pre-determined set of pinned linter versions to guarantee synchronicity
with CI
cd python/qemu && "make check" would do more or less the same as the
hacky script above, "make venv-check" would create a virtual environment
with precisely the right packages and then run the same.
I have been stalled there for a while, and I missed freeze deadline from
illness. Anguish. For now, the dumb script in my scripts directory does
the lifting I need it to, using packages I have selected in my local
environment that just-so-happen to pass.
Apropos qapi-gen testing scripts. I have scripts to show me how the
generated code changes along the way in a branch. They evolved over a
long time, and try to cope with changes in the tree that are hardly
relevant anymore. By now, they could quite possibly make Frankenstein
recoil in horror.
Are they in the tree? Largely if the generated code changes it's
invisible to me, but I rely heavily on the unit tests. I guess maybe if
they are not in a state to ups
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
John Snow writes:
> On 3/24/21 1:57 AM, Markus Armbruster wrote:
>> John Snow writes:
>>
>>> On 3/23/21 5:40 AM, Markus Armbruster wrote:
Naming rules differ for the various kinds of names. To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel(). For now, these merely
wrap around check_name_str(), but that will change shortly. Replace
the other uses of check_name_str() by appropriate uses of the
wrappers. No change in behavior just yet.
Signed-off-by: Markus Armbruster
---
scripts/qapi/expr.py | 51 +++-
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index e00467636c..30285fe334 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -21,11 +21,12 @@
from .error import QAPISemError
-# Names must be letters, numbers, -, and _. They must start
with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-'[a-zA-Z][a-zA-Z0-9_-]*$')
+# Names consist of letters, digits, -, and _, starting with a letter.
+# An experimental name is prefixed with x-. A name of a downstream
+# extension is prefixed with __RFQDN_. The latter prefix goes first.
+valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
+r'(x-)?'
+r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
def check_name_is_str(name, info, source):
@@ -37,16 +38,38 @@ def check_name_str(name, info, source,
permit_upper=False):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(name) or \
- c_name(name, False).startswith('q_'):
+match = valid_name.match(name)
+if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name" % source)
+return match.group(3)
+
+
+def check_name_upper(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[a-z-]' in @stem
+
>>>
>>> Creates (presumably) temporary errors in flake8 for the dead
>>> assignment here and below.
>>
>> All gone by the end of the series.
>>
>> "make check" and checkpatch were content. Anything else you'd like me
>> to run?
>
> Eventually it'll be part of CI, with targets to run locally.
>
> I never expected the process to take this long, so I did not invest my
> time in developing an interim solution.
>
> I use a hastily written script to do my own testing, which I run for
> every commit that touches QAPI:
>
> #!/usr/bin/env bash
> set -e
>
> if [[ -f qapi/.flake8 ]]; then
> echo "flake8 --config=qapi/.flake8 qapi/"
> flake8 --config=qapi/.flake8 qapi/
> fi
> if [[ -f qapi/pylintrc ]]; then
> echo "pylint --rcfile=qapi/pylintrc qapi/"
> pylint --rcfile=qapi/pylintrc qapi/
> fi
> if [[ -f qapi/mypy.ini ]]; then
> echo "mypy --config-file=qapi/mypy.ini qapi/"
> mypy --config-file=qapi/mypy.ini qapi/
> fi
>
> if [[ -f qapi/.isort.cfg ]]; then
> pushd qapi
> echo "isort -c ."
> isort -c .
> popd
> fi
>
> pushd ../bin/git
> make -j9
> make check-qapi-schema
> popd
Thanks for sharing this!
Apropos qapi-gen testing scripts. I have scripts to show me how the
generated code changes along the way in a branch. They evolved over a
long time, and try to cope with changes in the tree that are hardly
relevant anymore. By now, they could quite possibly make Frankenstein
recoil in horror.
As a secondary purpose, the scripts show me how output of pycodestyle-3
and pylint change. This would be uninteresting if the code in master
was clean against a useful configuration of these tools. Your work has
been making it less interesting.
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
On 3/24/21 1:57 AM, Markus Armbruster wrote:
John Snow writes:
On 3/23/21 5:40 AM, Markus Armbruster wrote:
Naming rules differ for the various kinds of names. To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel(). For now, these merely
wrap around check_name_str(), but that will change shortly. Replace
the other uses of check_name_str() by appropriate uses of the
wrappers. No change in behavior just yet.
Signed-off-by: Markus Armbruster
---
scripts/qapi/expr.py | 51 +++-
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index e00467636c..30285fe334 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -21,11 +21,12 @@
from .error import QAPISemError
-# Names must be letters, numbers, -, and _. They must start with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-'[a-zA-Z][a-zA-Z0-9_-]*$')
+# Names consist of letters, digits, -, and _, starting with a letter.
+# An experimental name is prefixed with x-. A name of a downstream
+# extension is prefixed with __RFQDN_. The latter prefix goes first.
+valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
+r'(x-)?'
+r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
def check_name_is_str(name, info, source):
@@ -37,16 +38,38 @@ def check_name_str(name, info, source,
permit_upper=False):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(name) or \
- c_name(name, False).startswith('q_'):
+match = valid_name.match(name)
+if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name" % source)
+return match.group(3)
+
+
+def check_name_upper(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[a-z-]' in @stem
+
Creates (presumably) temporary errors in flake8 for the dead
assignment here and below.
All gone by the end of the series.
"make check" and checkpatch were content. Anything else you'd like me
to run?
Eventually it'll be part of CI, with targets to run locally.
I never expected the process to take this long, so I did not invest my
time in developing an interim solution.
I use a hastily written script to do my own testing, which I run for
every commit that touches QAPI:
#!/usr/bin/env bash
set -e
if [[ -f qapi/.flake8 ]]; then
echo "flake8 --config=qapi/.flake8 qapi/"
flake8 --config=qapi/.flake8 qapi/
fi
if [[ -f qapi/pylintrc ]]; then
echo "pylint --rcfile=qapi/pylintrc qapi/"
pylint --rcfile=qapi/pylintrc qapi/
fi
if [[ -f qapi/mypy.ini ]]; then
echo "mypy --config-file=qapi/mypy.ini qapi/"
mypy --config-file=qapi/mypy.ini qapi/
fi
if [[ -f qapi/.isort.cfg ]]; then
pushd qapi
echo "isort -c ."
isort -c .
popd
fi
pushd ../bin/git
make -j9
make check-qapi-schema
popd
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
John Snow writes:
> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>> Naming rules differ for the various kinds of names. To prepare
>> enforcing them, define functions to check them: check_name_upper(),
>> check_name_lower(), and check_name_camel(). For now, these merely
>> wrap around check_name_str(), but that will change shortly. Replace
>> the other uses of check_name_str() by appropriate uses of the
>> wrappers. No change in behavior just yet.
>> Signed-off-by: Markus Armbruster
>> ---
>> scripts/qapi/expr.py | 51 +++-
>> 1 file changed, 36 insertions(+), 15 deletions(-)
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index e00467636c..30285fe334 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -21,11 +21,12 @@
>> from .error import QAPISemError
>>
>> -# Names must be letters, numbers, -, and _. They must start with letter,
>> -# except for downstream extensions which must start with __RFQDN_.
>> -# Dots are only valid in the downstream extension prefix.
>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>> -'[a-zA-Z][a-zA-Z0-9_-]*$')
>> +# Names consist of letters, digits, -, and _, starting with a letter.
>> +# An experimental name is prefixed with x-. A name of a downstream
>> +# extension is prefixed with __RFQDN_. The latter prefix goes first.
>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>> +r'(x-)?'
>> +r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>>
>> def check_name_is_str(name, info, source):
>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source,
>> permit_upper=False):
>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>> # and 'q_obj_*' implicit type names.
>> -if not valid_name.match(name) or \
>> - c_name(name, False).startswith('q_'):
>> +match = valid_name.match(name)
>> +if not match or c_name(name, False).startswith('q_'):
>> raise QAPISemError(info, "%s has an invalid name" % source)
>> if not permit_upper and name.lower() != name:
>> raise QAPISemError(
>> info, "%s uses uppercase in name" % source)
>> +return match.group(3)
>> +
>> +
>> +def check_name_upper(name, info, source):
>> +stem = check_name_str(name, info, source, permit_upper=True)
>> +# TODO reject '[a-z-]' in @stem
>> +
>
> Creates (presumably) temporary errors in flake8 for the dead
> assignment here and below.
All gone by the end of the series.
"make check" and checkpatch were content. Anything else you'd like me
to run?
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
On 3/23/21 5:40 AM, Markus Armbruster wrote:
Naming rules differ for the various kinds of names. To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel(). For now, these merely
wrap around check_name_str(), but that will change shortly. Replace
the other uses of check_name_str() by appropriate uses of the
wrappers. No change in behavior just yet.
Signed-off-by: Markus Armbruster
---
scripts/qapi/expr.py | 51 +++-
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index e00467636c..30285fe334 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -21,11 +21,12 @@
from .error import QAPISemError
-# Names must be letters, numbers, -, and _. They must start with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-'[a-zA-Z][a-zA-Z0-9_-]*$')
+# Names consist of letters, digits, -, and _, starting with a letter.
+# An experimental name is prefixed with x-. A name of a downstream
+# extension is prefixed with __RFQDN_. The latter prefix goes first.
+valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
+r'(x-)?'
+r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
def check_name_is_str(name, info, source):
@@ -37,16 +38,38 @@ def check_name_str(name, info, source,
permit_upper=False):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(name) or \
- c_name(name, False).startswith('q_'):
+match = valid_name.match(name)
+if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name" % source)
+return match.group(3)
+
+
+def check_name_upper(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[a-z-]' in @stem
+
Creates (presumably) temporary errors in flake8 for the dead assignment
here and below.
+
+def check_name_lower(name, info, source,
+ permit_upper=False):
+stem = check_name_str(name, info, source, permit_upper)
+# TODO reject '_' in stem
+
+
+def check_name_camel(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[_-]' in stem, require CamelCase
def check_defn_name_str(name, info, meta):
-check_name_str(name, info, meta, permit_upper=True)
+if meta == 'event':
+check_name_upper(name, info, meta)
+elif meta == 'command':
+check_name_lower(name, info, meta, permit_upper=True)
+else:
+check_name_camel(name, info, meta)
if name.endswith('Kind') or name.endswith('List'):
raise QAPISemError(
info, "%s name should not end in '%s'" % (meta, name[-4:]))
@@ -163,8 +186,7 @@ def check_type(value, info, source,
key_source = "%s member '%s'" % (source, key)
if key.startswith('*'):
key = key[1:]
-check_name_str(key, info, key_source,
- permit_upper=permit_upper)
+check_name_lower(key, info, key_source, permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
check_keys(arg, info, key_source, ['type'], ['if', 'features'])
@@ -186,7 +208,7 @@ def check_features(features, info):
check_keys(f, info, source, ['name'], ['if'])
check_name_is_str(f['name'], info, source)
source = "%s '%s'" % (source, f['name'])
-check_name_str(f['name'], info, source)
+check_name_lower(f['name'], info, source)
check_if(f, info, source)
@@ -213,8 +235,7 @@ def check_enum(expr, info):
# Enum members may start with a digit
if member_name[0].isdigit():
member_name = 'd' + member_name # Hack: hide the digit
-check_name_str(member_name, info, source,
- permit_upper=permit_upper)
+check_name_lower(member_name, info, source, permit_upper)
check_if(member, info, source)
@@ -244,7 +265,7 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
if discriminator is None:
-check_name_str(key, info, source)
+check_name_lower(key, info, source)
# else: name is in discriminator enum, which gets checked
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, sourc
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
Markus Armbruster writes: > Eric Blake writes: > >> On 3/23/21 4:40 AM, Markus Armbruster wrote: >>> Naming rules differ for the various kinds of names. To prepare >>> enforcing them, define functions to check them: check_name_upper(), >>> check_name_lower(), and check_name_camel(). For now, these merely >>> wrap around check_name_str(), but that will change shortly. Replace >>> the other uses of check_name_str() by appropriate uses of the >>> wrappers. No change in behavior just yet. >>> >>> Signed-off-by: Markus Armbruster >>> --- >>> scripts/qapi/expr.py | 51 +++- >>> 1 file changed, 36 insertions(+), 15 deletions(-) >>> >> >>> +++ b/scripts/qapi/expr.py >>> @@ -21,11 +21,12 @@ >>> from .error import QAPISemError >>> >>> >>> -# Names must be letters, numbers, -, and _. They must start with letter, >>> -# except for downstream extensions which must start with __RFQDN_. >>> -# Dots are only valid in the downstream extension prefix. >>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >>> -'[a-zA-Z][a-zA-Z0-9_-]*$') >> >> I'm assuming python concatenates r'' with '' in the obvious manner... >> >>> +# Names consist of letters, digits, -, and _, starting with a letter. >>> +# An experimental name is prefixed with x-. A name of a downstream >>> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >>> +r'(x-)?' >>> +r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) >> >> ...but like your explicit use of r'' r''. >> >> Splitting out special handling of r'(x-)?' does not change behavior, but >> is not otherwise mentioned in your commit message. I suspect you did it >> to make it easier to permit x-EVENT_NAME in later patches where upper is >> handled differently from lower or camel, > > Yes. > >> so I won't withhold R-b, but it >> may be worth a tweak to the commit message. > > Probably. I'm failing at coming up with a concise text that isn't > confusing. Adding this paragraph: check_name_str() now returns the name without downstream and x- prefix, for use by the wrappers in later patches. Requires tweaking regexp @valid_name. It accepts the same strings as before.
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
Eric Blake writes: > On 3/23/21 4:40 AM, Markus Armbruster wrote: >> Naming rules differ for the various kinds of names. To prepare >> enforcing them, define functions to check them: check_name_upper(), >> check_name_lower(), and check_name_camel(). For now, these merely >> wrap around check_name_str(), but that will change shortly. Replace >> the other uses of check_name_str() by appropriate uses of the >> wrappers. No change in behavior just yet. >> >> Signed-off-by: Markus Armbruster >> --- >> scripts/qapi/expr.py | 51 +++- >> 1 file changed, 36 insertions(+), 15 deletions(-) >> > >> +++ b/scripts/qapi/expr.py >> @@ -21,11 +21,12 @@ >> from .error import QAPISemError >> >> >> -# Names must be letters, numbers, -, and _. They must start with letter, >> -# except for downstream extensions which must start with __RFQDN_. >> -# Dots are only valid in the downstream extension prefix. >> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >> -'[a-zA-Z][a-zA-Z0-9_-]*$') > > I'm assuming python concatenates r'' with '' in the obvious manner... > >> +# Names consist of letters, digits, -, and _, starting with a letter. >> +# An experimental name is prefixed with x-. A name of a downstream >> +# extension is prefixed with __RFQDN_. The latter prefix goes first. >> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' >> +r'(x-)?' >> +r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) > > ...but like your explicit use of r'' r''. > > Splitting out special handling of r'(x-)?' does not change behavior, but > is not otherwise mentioned in your commit message. I suspect you did it > to make it easier to permit x-EVENT_NAME in later patches where upper is > handled differently from lower or camel, Yes. > so I won't withhold R-b, but it > may be worth a tweak to the commit message. Probably. I'm failing at coming up with a concise text that isn't confusing. >> def check_defn_name_str(name, info, meta): >> -check_name_str(name, info, meta, permit_upper=True) >> +if meta == 'event': >> +check_name_upper(name, info, meta) >> +elif meta == 'command': >> +check_name_lower(name, info, meta, permit_upper=True) > > Why do commands need to permit upper? I guess just downstream FQDN > extensions? This is just so that the patch doesn't change behavior. PATCH 24 will flip it to False. > Otherwise the patch makes sense. > > Reviewed-by: Eric Blake Thanks!
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
On 3/23/21 10:20 AM, Eric Blake wrote: -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' -'[a-zA-Z][a-zA-Z0-9_-]*$') I'm assuming python concatenates r'' with '' in the obvious manner... FWIW, I don't think it does, actually. I believe you do need to spell out each individual string constant with what type it is. (In this case, missing the second r has no effect as there are no backslash sequences in the string.) --js
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
On 3/23/21 9:30 AM, John Snow wrote: > On 3/23/21 10:20 AM, Eric Blake wrote: >>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' >>> - '[a-zA-Z][a-zA-Z0-9_-]*$') >> I'm assuming python concatenates r'' with '' in the obvious manner... >> > > FWIW, I don't think it does, actually. I believe you do need to spell > out each individual string constant with what type it is. > > (In this case, missing the second r has no effect as there are no > backslash sequences in the string.) Aha - https://docs.python.org/3/reference/lexical_analysis.html#string-literal-concatenation talks about it more, and even mentions that joining r'' with plain '' is useful for scenarios where you want easier use of \ through only part of your overall literal (since string literal concatenation is performed at compile time). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
On 3/23/21 4:40 AM, Markus Armbruster wrote: > Naming rules differ for the various kinds of names. To prepare > enforcing them, define functions to check them: check_name_upper(), > check_name_lower(), and check_name_camel(). For now, these merely > wrap around check_name_str(), but that will change shortly. Replace > the other uses of check_name_str() by appropriate uses of the > wrappers. No change in behavior just yet. > > Signed-off-by: Markus Armbruster > --- > scripts/qapi/expr.py | 51 +++- > 1 file changed, 36 insertions(+), 15 deletions(-) > > +++ b/scripts/qapi/expr.py > @@ -21,11 +21,12 @@ > from .error import QAPISemError > > > -# Names must be letters, numbers, -, and _. They must start with letter, > -# except for downstream extensions which must start with __RFQDN_. > -# Dots are only valid in the downstream extension prefix. > -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' > -'[a-zA-Z][a-zA-Z0-9_-]*$') I'm assuming python concatenates r'' with '' in the obvious manner... > +# Names consist of letters, digits, -, and _, starting with a letter. > +# An experimental name is prefixed with x-. A name of a downstream > +# extension is prefixed with __RFQDN_. The latter prefix goes first. > +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' > +r'(x-)?' > +r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) ...but like your explicit use of r'' r''. Splitting out special handling of r'(x-)?' does not change behavior, but is not otherwise mentioned in your commit message. I suspect you did it to make it easier to permit x-EVENT_NAME in later patches where upper is handled differently from lower or camel, so I won't withhold R-b, but it may be worth a tweak to the commit message. > > def check_defn_name_str(name, info, meta): > -check_name_str(name, info, meta, permit_upper=True) > +if meta == 'event': > +check_name_upper(name, info, meta) > +elif meta == 'command': > +check_name_lower(name, info, meta, permit_upper=True) Why do commands need to permit upper? I guess just downstream FQDN extensions? Otherwise the patch makes sense. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 10/28] qapi: Rework name checking in preparation of stricter checking
Naming rules differ for the various kinds of names. To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel(). For now, these merely
wrap around check_name_str(), but that will change shortly. Replace
the other uses of check_name_str() by appropriate uses of the
wrappers. No change in behavior just yet.
Signed-off-by: Markus Armbruster
---
scripts/qapi/expr.py | 51 +++-
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index e00467636c..30285fe334 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -21,11 +21,12 @@
from .error import QAPISemError
-# Names must be letters, numbers, -, and _. They must start with letter,
-# except for downstream extensions which must start with __RFQDN_.
-# Dots are only valid in the downstream extension prefix.
-valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
-'[a-zA-Z][a-zA-Z0-9_-]*$')
+# Names consist of letters, digits, -, and _, starting with a letter.
+# An experimental name is prefixed with x-. A name of a downstream
+# extension is prefixed with __RFQDN_. The latter prefix goes first.
+valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
+r'(x-)?'
+r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
def check_name_is_str(name, info, source):
@@ -37,16 +38,38 @@ def check_name_str(name, info, source,
permit_upper=False):
# Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
# and 'q_obj_*' implicit type names.
-if not valid_name.match(name) or \
- c_name(name, False).startswith('q_'):
+match = valid_name.match(name)
+if not match or c_name(name, False).startswith('q_'):
raise QAPISemError(info, "%s has an invalid name" % source)
if not permit_upper and name.lower() != name:
raise QAPISemError(
info, "%s uses uppercase in name" % source)
+return match.group(3)
+
+
+def check_name_upper(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[a-z-]' in @stem
+
+
+def check_name_lower(name, info, source,
+ permit_upper=False):
+stem = check_name_str(name, info, source, permit_upper)
+# TODO reject '_' in stem
+
+
+def check_name_camel(name, info, source):
+stem = check_name_str(name, info, source, permit_upper=True)
+# TODO reject '[_-]' in stem, require CamelCase
def check_defn_name_str(name, info, meta):
-check_name_str(name, info, meta, permit_upper=True)
+if meta == 'event':
+check_name_upper(name, info, meta)
+elif meta == 'command':
+check_name_lower(name, info, meta, permit_upper=True)
+else:
+check_name_camel(name, info, meta)
if name.endswith('Kind') or name.endswith('List'):
raise QAPISemError(
info, "%s name should not end in '%s'" % (meta, name[-4:]))
@@ -163,8 +186,7 @@ def check_type(value, info, source,
key_source = "%s member '%s'" % (source, key)
if key.startswith('*'):
key = key[1:]
-check_name_str(key, info, key_source,
- permit_upper=permit_upper)
+check_name_lower(key, info, key_source, permit_upper)
if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
raise QAPISemError(info, "%s uses reserved name" % key_source)
check_keys(arg, info, key_source, ['type'], ['if', 'features'])
@@ -186,7 +208,7 @@ def check_features(features, info):
check_keys(f, info, source, ['name'], ['if'])
check_name_is_str(f['name'], info, source)
source = "%s '%s'" % (source, f['name'])
-check_name_str(f['name'], info, source)
+check_name_lower(f['name'], info, source)
check_if(f, info, source)
@@ -213,8 +235,7 @@ def check_enum(expr, info):
# Enum members may start with a digit
if member_name[0].isdigit():
member_name = 'd' + member_name # Hack: hide the digit
-check_name_str(member_name, info, source,
- permit_upper=permit_upper)
+check_name_lower(member_name, info, source, permit_upper)
check_if(member, info, source)
@@ -244,7 +265,7 @@ def check_union(expr, info):
for (key, value) in members.items():
source = "'data' member '%s'" % key
if discriminator is None:
-check_name_str(key, info, source)
+check_name_lower(key, info, source)
# else: name is in discriminator enum, which gets checked
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
@@ -258,7 +279,7 @@ def check_alternate(expr, info):
raise QAPISemError(info, "'data' must not be empty")
for (key, value) in members.items():
source = "'data' membe
