qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] fixup! qapi: Forbid case-insensitive clashes


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] fixup! qapi: Forbid case-insensitive clashes
Date: Thu, 19 Nov 2015 14:32:45 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> writes:

> [Replace the old commit message with this:]
>
> qapi: Forbid case-insensitive clashes
>
> In general, designing user interfaces that rely on case
> distinction is poor practice.  Another benefit of enforcing
> a restriction against case-insensitive clashes is that we
> no longer have to worry about the situation of enum values
> that could be distinguished by case if mapped by c_name(),
> but which cannot be distinguished when mapped to C as
> ALL_CAPS by c_enum_const() [via c_name(name, False).upper()].
> Thus, having the generator look for case collisions up front
> will prevent developers from worrying whether different
> munging rules for member names compared to enum values as a
> discriminator will cause any problems in qapi unions.
>
> We do not have to care about c_name(name, True) vs.
> c_name(name, False), as long as any pair of munged names being
> compared were munged using the same flag value to c_name().
> This is because commit 9fb081e already reserved names munging
> to 'q_', and this patch extends the reservation to 'Q_'; and
> because recent patches fixed things to ensure the only thing
> done by the flag is adding the prefix 'q_', that the only use
> of '.' (which also munges to '_') is in downstream extension
> prefixes, and that enum munging does not add underscores to
> any CamelCase values.
>
> However, we DO have to care about the fact that we have a
> command 'stop' and an event 'STOP' (currently the only case
> collision of any of our .json entities).  To solve that, add
> a new QAPISchemaEntity.c_namecase() that returns a munged
> version of the name that can be used in checking for case
> clashes,

For [-._] clashes, too.  Easiest fix it to just say "checking for
clashes".

>          and override it in QAPISchemaEvent to use a different
> case (thus, indexing by .c_namecase() will create two natural
> key partitions), then add a new _clash_dict to
> QAPISchema._def_entity() that checks for case collisions after

To QAPISchema, actually.  In _def_entity(), we only add an entry.

> ruling out identical spellings.  This works because all
> entities have at least a leading letter in their name.
>
> We could go further and enforce that events are named with
> 'ALL_CAPS' and that nothing else is named in that manner; but
> that can be done as a followup if desired.  We could also
> separate commands from type names (the way we already separated
> events into a separate namespace), but then we'd have to make
> .c_namecase() pick something other than the convenient upper()
> vs. lower() in order to partition dictionary keys into three
> sets (perhaps a leading digit, since all entities start with a
> letter).

The upper / lower partition works for events / anything else, and is
kind of cute, although two separate clash dictionaries might be clearer.
I'd like to sketch that next, to help us decide.

Even if we decide to stick to the "partition the key space" idea now,
I'm reluctant to push it farther.  Prepending a digit would feel odd.
Title case would be slightly better.  But I think three separate clash
dictionaries would be simpler and clearer then.

Depending on what we do, the paragraph above may need some more
tweaking.

> There is also the possibility that we may want to add a future
> extension to QMP of teaching it to be case-insensitive (the
> user could request command 'Quit' instead of 'quit', or could
> spell a struct field as 'CPU' instead of 'cpu').  But for that
> to be a practical extension, we cannot break backwards
> compatibility with any existing struct that was already
> relying on case sensitivity.  Fortunately, after a recent
> patch cleaned up CpuInfo, there are no such existing qapi
> structs.  Of course, the idea of a future extension is not
> as strong of a reason to make the change.
>
> At any rate, it is easier to be strict now, and relax things
> later if we find a reason to need case-sensitive QMP members,
> than it would be to wish we had the restriction in place.
>
> Add new tests args-case-clash.json and command-type-case-clash.json
> to demonstrate that the collision detection works.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> Use a separate clash dictionary, fix a typo that we couldn't
> trigger, add .c_namecase(), no longer need to use attrgetter
> ---
>  scripts/qapi.py | 48 ++++++++++++++++++------------------------------
>  1 file changed, 18 insertions(+), 30 deletions(-)

I find the squashed patch easier to understand, so here it is:

| diff --git a/scripts/qapi.py b/scripts/qapi.py
| index 72925c3..114e07a 100644
| --- a/scripts/qapi.py
| +++ b/scripts/qapi.py
| @@ -378,9 +378,9 @@ def check_name(expr_info, source, name, 
allow_optional=False,
|      # code always prefixes it with the enum name
|      if enum_member and membername[0].isdigit():
|          membername = 'D' + membername
| -    # Reserve the entire 'q_' namespace for c_name()
| +    # Reserve the entire 'q_'/'Q_' namespace for c_name()
|      if not valid_name.match(membername) or \
| -       c_name(membername, False).startswith('q_'):
| +       c_name(membername, False).lower().startswith('q_'):
|          raise QAPIExprError(expr_info,
|                              "%s uses invalid name '%s'" % (source, name))
|  
| @@ -800,6 +800,9 @@ class QAPISchemaEntity(object):
|      def c_name(self):
|          return c_name(self.name)
|  
| +    def c_namecase(self):
| +        return c_name(self.name).lower()
| +
|      def check(self, schema):
|          pass
|  

The entities are: commands, events, the various types.  This
implementation of c_namecase() covers commands and types.  It gets
overwritten for events.

Commands should not use upper case.  If we enforced that, the .lower()
would be superfluous there.  But it isn't wrong.

| @@ -1040,7 +1043,7 @@ class QAPISchemaObjectTypeMember(object):
|          assert self.type
|  
|      def check_clash(self, info, seen):
| -        cname = c_name(self.name)
| +        cname = c_name(self.name).lower()
|          if cname in seen:
|              raise QAPIExprError(info,
|                                  "%s collides with %s"

Not an entity, therefore QAPISchemaEntity.c_namecase() isn't inherited.

Members should not use upper case.  If we enforced that, the .lower()
would be superfluous.

| @@ -1085,7 +1088,7 @@ class QAPISchemaObjectTypeVariants(object):
|  
|      def check(self, schema, seen):
|          if not self.tag_member:    # flat union
| -            self.tag_member = seen[c_name(self.tag_name)]
| +            self.tag_member = seen[c_name(self.tag_name).lower()]
|              assert self.tag_name == self.tag_member.name
|          assert isinstance(self.tag_member.type, QAPISchemaEnumType)
|          for v in self.variants:

Key needs to match whatever we use in
QAPISchemaObjectTypeMember.check_clash().  It does.

| @@ -1184,12 +1187,16 @@ class QAPISchemaEvent(QAPISchemaEntity):
|      def visit(self, visitor):
|          visitor.visit_event(self.name, self.info, self.arg_type)
|  
| +    def c_namecase(self):
| +        return c_name(self.name).upper()
| +

Event names should not use lower case.  If we enforced that, the
.upper() would be superfluous.

If we enforced our naming conventions, namely types in CamelCase, events
in ALL_CAPS, everything else lower-case, the c_namecase() would become
slightly simpler: it's same as c_name() except for types, where it
additionally folds to lower case.  The name would be misleading then,
however.  Anyway, we can consider that once we enforce naming
conventions.

|  
|  class QAPISchema(object):
|      def __init__(self, fname):
|          try:
|              self.exprs = check_exprs(QAPISchemaParser(open(fname, 
"r")).exprs)
|              self._entity_dict = {}
| +            self._clash_dict = {}
|              self._predefining = True
|              self._def_predefineds()
|              self._predefining = False
| @@ -1203,7 +1210,13 @@ class QAPISchema(object):
|          # Only the predefined types are allowed to not have info
|          assert ent.info or self._predefining
|          assert ent.name not in self._entity_dict
| +        cname = ent.c_namecase()
| +        if cname in self._clash_dict:
| +            raise QAPIExprError(ent.info,
| +                                "Entity '%s' collides with '%s'"
| +                                % (ent.name, self._clash_dict[cname]))
|          self._entity_dict[ent.name] = ent
| +        self._clash_dict[cname] = ent.name
|  
|      def lookup_entity(self, name, typ=None):
|          ent = self._entity_dict.get(name)




reply via email to

[Prev in Thread] Current Thread [Next in Thread]