Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/common.py | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index dce289ae21..cc5a5941dd 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -290,8 +290,12 @@ class QAPISchemaParser(object):
                 if not isinstance(include, str):
                     raise QAPISemError(info,
                                        "Value of 'include' must be a string")
-                self._include(include, info, os.path.dirname(self.fname),
-                              previously_included)
+                exprs_include = self._include(include, info,
+                                              os.path.dirname(self.fname),
+                                              previously_included)
+                if exprs_include:
+                    self.exprs.extend(exprs_include.exprs)
+                    self.docs.extend(exprs_include.docs)
             elif "pragma" in expr:
                 self.reject_expr_doc(cur_doc)
                 if len(expr) != 1:
@@ -334,14 +338,13 @@ class QAPISchemaParser(object):
 
         # skip multiple include of the same file
         if incl_abs_fname in previously_included:
-            return
+            return None
+
         try:
             fobj = open(incl_fname, 'r')
         except IOError as e:
             raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
-        exprs_include = QAPISchemaParser(fobj, previously_included, info)
-        self.exprs.extend(exprs_include.exprs)
-        self.docs.extend(exprs_include.docs)
+        return QAPISchemaParser(fobj, previously_included, info)
 
     def _pragma(self, name, value, info):
         global doc_required, returns_whitelist, name_case_whitelist
-- 
2.13.6

On 02/11/2018 03:35 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   scripts/qapi/common.py | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)

Again, not much mention of 'why' in the commit message.  But 
centralizing the initialization operations in one place rather than 
having to chase down multiple places is good.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Quoting Markus Armbruster (2018-02-11 03:35:52)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/common.py | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index dce289ae21..cc5a5941dd 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -290,8 +290,12 @@ class QAPISchemaParser(object):
>                  if not isinstance(include, str):
>                      raise QAPISemError(info,
>                                         "Value of 'include' must be a string")
> -                self._include(include, info, os.path.dirname(self.fname),
> -                              previously_included)
> +                exprs_include = self._include(include, info,
> +                                              os.path.dirname(self.fname),
> +                                              previously_included)
> +                if exprs_include:
> +                    self.exprs.extend(exprs_include.exprs)
> +                    self.docs.extend(exprs_include.docs)
>              elif "pragma" in expr:
>                  self.reject_expr_doc(cur_doc)
>                  if len(expr) != 1:
> @@ -334,14 +338,13 @@ class QAPISchemaParser(object):
> 
>          # skip multiple include of the same file
>          if incl_abs_fname in previously_included:
> -            return
> +            return None
> +
>          try:
>              fobj = open(incl_fname, 'r')
>          except IOError as e:
>              raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
> -        exprs_include = QAPISchemaParser(fobj, previously_included, info)
> -        self.exprs.extend(exprs_include.exprs)
> -        self.docs.extend(exprs_include.docs)

minor nit, but the function of _include() seems more appropriately
described now as _parse_include() or _parse_if_new() or something similar.
Maybe it would be more readable to just move the previously_included
checks into init() and just drop _include() entirely?

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> +        return QAPISchemaParser(fobj, previously_included, info)
> 
>      def _pragma(self, name, value, info):
>          global doc_required, returns_whitelist, name_case_whitelist
> -- 
> 2.13.6
> 

Michael Roth <mdroth@linux.vnet.ibm.com> writes:

> Quoting Markus Armbruster (2018-02-11 03:35:52)
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  scripts/qapi/common.py | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index dce289ae21..cc5a5941dd 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -290,8 +290,12 @@ class QAPISchemaParser(object):
>>                  if not isinstance(include, str):
>>                      raise QAPISemError(info,
>>                                         "Value of 'include' must be a string")
>> -                self._include(include, info, os.path.dirname(self.fname),
>> -                              previously_included)
>> +                exprs_include = self._include(include, info,
>> +                                              os.path.dirname(self.fname),
>> +                                              previously_included)
>> +                if exprs_include:
>> +                    self.exprs.extend(exprs_include.exprs)
>> +                    self.docs.extend(exprs_include.docs)
>>              elif "pragma" in expr:
>>                  self.reject_expr_doc(cur_doc)
>>                  if len(expr) != 1:
>> @@ -334,14 +338,13 @@ class QAPISchemaParser(object):
>> 
>>          # skip multiple include of the same file
>>          if incl_abs_fname in previously_included:
>> -            return
>> +            return None
>> +
>>          try:
>>              fobj = open(incl_fname, 'r')
>>          except IOError as e:
>>              raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
>> -        exprs_include = QAPISchemaParser(fobj, previously_included, info)
>> -        self.exprs.extend(exprs_include.exprs)
>> -        self.docs.extend(exprs_include.docs)
>
> minor nit, but the function of _include() seems more appropriately
> described now as _parse_include() or _parse_if_new() or something similar.

I'm open to renaming, but "parse" doesn't really fit.  _include()'s
caller checks syntax, then calls _include() to check semantic
constraints and evaluate the include, then splices in the evaluated
result.

> Maybe it would be more readable to just move the previously_included
> checks into init() and just drop _include() entirely?

Maybe.  But the conditional gets rather long then.

> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Thanks!