Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Sep 25, 2020 at 11:15:28AM -0400, Eduardo Habkost wrote:
>> On Fri, Sep 25, 2020 at 03:00:51PM +0200, Markus Armbruster wrote:
>> > Eduardo Habkost <ehabkost@redhat.com> writes:
>> > 
>> > > On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
>> > >> The edge case is that if the name is '', this expression returns a
>> > >> string instead of a bool, which violates our declared type.
>> > >> 
>> > >> Signed-off-by: John Snow <jsnow@redhat.com>
>> > >> ---
>> > >>  scripts/qapi/gen.py | 2 +-
>> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >> 
>> > >> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>> > >> index 9898d513ae..cb2b2655c3 100644
>> > >> --- a/scripts/qapi/gen.py
>> > >> +++ b/scripts/qapi/gen.py
>> > >> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, 
>> > >> builtin_blurb, pydoc):
>> > >>  
>> > >>      @staticmethod
>> > >>      def _is_user_module(name):
>> > >> -        return name and not name.startswith('./')
>> > >> +        return name is not None and not name.startswith('./')
>> > >
>> > > This changes behavior if name=='', and I guess this is OK, but
>> > > I'm not sure.
>> > 
>> > @name is either
>> > 
>> > (1) A module pathname relative to the main module
>> > 
>> >     This is a module defined by the user.
>> > 
>> > (2) system module name, starting with './'
>> > 
>> >     This is a named system module.  We currently have two: './init' in
>> >     commands.py, and and './emit' in events.py.
>> > 
>> > (3) None
>> > 
>> >     This is the (nameless) system module for built-in stuff.  It
>> >     predates (2).  Using './builtin' would probably be better now.
>> > 
>> > Note that (1) and (2) are disjoint: relative pathnames do not begin with
>> > './'.
>> > 
>> > name='' is not possible, because '' is not a valid pathname.
>> 
>> Thanks!  So, the './' prefix is just internal state and never
>> visible to the outside, correct?

Yes.

>>                                   I would use a separate bool
>> instead of trying to encode additional state inside the string.
>
> I've found only one place where the './' prefix might be leaking,
> and I don't know if it's intentional or not:
>
> Is the name argument to visit_include() supposed to be always
> (1), or are './' pathnames allowed too?

Always (1).

visit_include() gets passed a module name:

class QAPISchemaInclude(QAPISchemaEntity):
    [...]
    def visit(self, visitor):
        super().visit(visitor)
        visitor.visit_include(self._sub_module.name, self.info)

Module names are relative to the main module's directory:

    def _module_name(self, fname):
        if fname is None:
            return None
        return os.path.relpath(fname, self._schema_dir)

os,path.relpath() normalizes away './':

    $ python
    Python 3.8.5 (default, Aug 12 2020, 00:00:00) 
    [GCC 10.2.1 20200723 (Red Hat 10.2.1-1)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> os.path.relpath('./sub.json', '')
    'sub.json'

QAPISchema._make_module() uses ._module_name() as it should:

    def _make_module(self, fname):
        name = self._module_name(fname)
        if name not in self._module_dict:
            self._module_dict[name] = QAPISchemaModule(name)
        return self._module_dict[name]