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

[Eduardo Habkost]

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?  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?

-- 
Eduardo