| Message ID | 20210323094025.3569441-7-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: Enforce naming rules | expand |
On 3/23/21 5:40 AM, Markus Armbruster wrote: > Member name 'u' and names starting with 'has-' or 'has_' are reserved > for the generator. check_type() enforces this, covered by tests > reserved-member-u and reserved-member-has. > > These tests neglect to cover optional members, where the name starts > with '*'. Tweak reserved-member-u to fix that. > > This demonstrates the reserved member name check is broken for > optional members. The next commit will fix it. > The test without an optional member goes away. Do we lose coverage? (Do we care?) > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > tests/qapi-schema/reserved-member-u.err | 2 -- > tests/qapi-schema/reserved-member-u.json | 3 ++- > tests/qapi-schema/reserved-member-u.out | 14 ++++++++++++++ > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err > index 231d552494..e69de29bb2 100644 > --- a/tests/qapi-schema/reserved-member-u.err > +++ b/tests/qapi-schema/reserved-member-u.err > @@ -1,2 +0,0 @@ > -reserved-member-u.json: In struct 'Oops': > -reserved-member-u.json:7: 'data' member 'u' uses reserved name > diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json > index 1eaf0f301c..15005abb09 100644 > --- a/tests/qapi-schema/reserved-member-u.json > +++ b/tests/qapi-schema/reserved-member-u.json > @@ -4,4 +4,5 @@ > # This is true even for non-unions, because it is possible to convert a > # struct to flat union while remaining backwards compatible in QMP. > # TODO - we could munge the member name to 'q_u' to avoid the collision > -{ 'struct': 'Oops', 'data': { 'u': 'str' } } > +# BUG: not rejected > +{ 'struct': 'Oops', 'data': { '*u': 'str' } } > diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out > index e69de29bb2..6a3705518b 100644 > --- a/tests/qapi-schema/reserved-member-u.out > +++ b/tests/qapi-schema/reserved-member-u.out > @@ -0,0 +1,14 @@ > +module ./builtin > +object q_empty > +enum QType > + prefix QTYPE > + member none > + member qnull > + member qnum > + member qstring > + member qdict > + member qlist > + member qbool > +module reserved-member-u.json > +object Oops > + member u: str optional=True >
John Snow <jsnow@redhat.com> writes: > On 3/23/21 5:40 AM, Markus Armbruster wrote: >> Member name 'u' and names starting with 'has-' or 'has_' are reserved >> for the generator. check_type() enforces this, covered by tests >> reserved-member-u and reserved-member-has. >> These tests neglect to cover optional members, where the name starts >> with '*'. Tweak reserved-member-u to fix that. >> This demonstrates the reserved member name check is broken for >> optional members. The next commit will fix it. >> > > The test without an optional member goes away. Do we lose coverage? > (Do we care?) Up to a point :) We do try to cover all failure modes, just not in all contexts. The test is about this error: if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError(info, "%s uses reserved name" % key_source) Full matrix: (is "u", starts with "has_") x (optional, not optional). Instead of covering all four cases, we cover two: non-optional "u" (reserved-member-u) and non-optional "has-" (reserved-member-has). The patch flips the former to optional. The latter still covers non-optional. Good enough, I think. Do you feel I should point to reserved-member-has in the commit message?
On 3/23/21 11:44 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 3/23/21 5:40 AM, Markus Armbruster wrote: >>> Member name 'u' and names starting with 'has-' or 'has_' are reserved >>> for the generator. check_type() enforces this, covered by tests >>> reserved-member-u and reserved-member-has. >>> These tests neglect to cover optional members, where the name starts >>> with '*'. Tweak reserved-member-u to fix that. >>> This demonstrates the reserved member name check is broken for >>> optional members. The next commit will fix it. >>> >> >> The test without an optional member goes away. Do we lose coverage? >> (Do we care?) > > Up to a point :) We do try to cover all failure modes, just not in all > contexts. > > The test is about this error: > > if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): > raise QAPISemError(info, "%s uses reserved name" % key_source) > > Full matrix: (is "u", starts with "has_") x (optional, not optional). > > Instead of covering all four cases, we cover two: non-optional "u" > (reserved-member-u) and non-optional "has-" (reserved-member-has). > > The patch flips the former to optional. The latter still covers > non-optional. > > Good enough, I think. > Relies a tiny bit on knowing these two reserved name checks are implemented in the same place. Doubt it'll matter practically. Coverage has increased overall. > Do you feel I should point to reserved-member-has in the commit message? > It'd be for my benefit, but you also already just explained it to me. Reviewed-by: John Snow <jsnow@redhat.com>
John Snow <jsnow@redhat.com> writes: > On 3/23/21 11:44 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 3/23/21 5:40 AM, Markus Armbruster wrote: >>>> Member name 'u' and names starting with 'has-' or 'has_' are reserved >>>> for the generator. check_type() enforces this, covered by tests >>>> reserved-member-u and reserved-member-has. >>>> These tests neglect to cover optional members, where the name starts >>>> with '*'. Tweak reserved-member-u to fix that. >>>> This demonstrates the reserved member name check is broken for >>>> optional members. The next commit will fix it. >>>> >>> >>> The test without an optional member goes away. Do we lose coverage? >>> (Do we care?) >> Up to a point :) We do try to cover all failure modes, just not in >> all >> contexts. >> The test is about this error: >> if c_name(key, False) == 'u' or c_name(key, >> False).startswith('has_'): >> raise QAPISemError(info, "%s uses reserved name" % key_source) >> Full matrix: (is "u", starts with "has_") x (optional, not >> optional). >> Instead of covering all four cases, we cover two: non-optional "u" >> (reserved-member-u) and non-optional "has-" (reserved-member-has). >> The patch flips the former to optional. The latter still covers >> non-optional. >> Good enough, I think. >> > > Relies a tiny bit on knowing these two reserved name checks are > implemented in the same place. Doubt it'll matter > practically. Coverage has increased overall. > >> Do you feel I should point to reserved-member-has in the commit message? >> > > It'd be for my benefit, but you also already just explained it to me. Amending the second paragraph: These tests neglect to cover optional members, where the name starts with '*'. Tweak reserved-member-u to fix that. Test reserved-member-has still covers non-optional members. > Reviewed-by: John Snow <jsnow@redhat.com> Thanks!
diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err index 231d552494..e69de29bb2 100644 --- a/tests/qapi-schema/reserved-member-u.err +++ b/tests/qapi-schema/reserved-member-u.err @@ -1,2 +0,0 @@ -reserved-member-u.json: In struct 'Oops': -reserved-member-u.json:7: 'data' member 'u' uses reserved name diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json index 1eaf0f301c..15005abb09 100644 --- a/tests/qapi-schema/reserved-member-u.json +++ b/tests/qapi-schema/reserved-member-u.json @@ -4,4 +4,5 @@ # This is true even for non-unions, because it is possible to convert a # struct to flat union while remaining backwards compatible in QMP. # TODO - we could munge the member name to 'q_u' to avoid the collision -{ 'struct': 'Oops', 'data': { 'u': 'str' } } +# BUG: not rejected +{ 'struct': 'Oops', 'data': { '*u': 'str' } } diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out index e69de29bb2..6a3705518b 100644 --- a/tests/qapi-schema/reserved-member-u.out +++ b/tests/qapi-schema/reserved-member-u.out @@ -0,0 +1,14 @@ +module ./builtin +object q_empty +enum QType + prefix QTYPE + member none + member qnull + member qnum + member qstring + member qdict + member qlist + member qbool +module reserved-member-u.json +object Oops + member u: str optional=True
Member name 'u' and names starting with 'has-' or 'has_' are reserved for the generator. check_type() enforces this, covered by tests reserved-member-u and reserved-member-has. These tests neglect to cover optional members, where the name starts with '*'. Tweak reserved-member-u to fix that. This demonstrates the reserved member name check is broken for optional members. The next commit will fix it. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/qapi-schema/reserved-member-u.err | 2 -- tests/qapi-schema/reserved-member-u.json | 3 ++- tests/qapi-schema/reserved-member-u.out | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 3 deletions(-)