Eduardo Habkost <ehabk...@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 <ehabk...@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 <js...@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]
