QAPI patches patches for 2021-05-20

-----BEGIN PGP SIGNATURE-----
 
 iQJGBAABCAAwFiEENUvIs9frKmtoZ05fOHC0AOuRhlMFAmCme90SHGFybWJydUBy
 ZWRoYXQuY29tAAoJEDhwtADrkYZTnIcP/1XUG8dn8jI57s1D4Dq9XUgyFYHAK7oZ
 gNwY9uzlNWxJLpVQthSuOlUS/9f50/xc7wEoRpuAYR8v+480bpu77FEy6NAD+KH3
 yO1iSlHZtivSzNvpLWxj5vGUZE4SOWRyUiEBrBXcwhZ2YCz/FsxtGLK5heCubPQw
 QFGg70FaFrblZZp6RCUp3O/OLNG93DptrhM7Mcr6XeUOyU884pgSZwodjyoYE8KZ
 bwiYgppiiydeFexZgyyJ7+IWREAlb/2bOCIgS3bziaQBJHJPAkteRRCt1BKCv4F0
 q4blIiF8TsNQ/oqVM0KaX8WsbU2F3Ci4+RWyzJCNHi4Ickf9tcNBJ/RMRiUIOk9U
 A6zIGhm/L6g8h7ia1avfgqjGmoZIA6lUn1GEhudoQcgagM3hPbrFdZoPLDtfi639
 bher5gTPHyVb2B/xpqKEoek2bRKtpIGPdHzoCsvttQuUh787dM7QbPOPJZ8tHrCu
 uLmRIXg64BZZ7kzSxmrYiN2Z2ptY9+xxmBOds2cm3jO6DjCEC9LEe583Gcn+yf5N
 MKo72SEv9/ctyXs8CvNtBDjzabltMg7qwJCqmna9r6PdGh2rR6jVMzUWXsZDsc2Z
 lCrn7srlG4TkeTSr0o7pmqdWjWgr7ryQlbF2Fp2EpZQBh4KyBY21EHs5mOJBp8Mp
 aSEgdeyO+l9+
 =GxcF
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2021-05-20' into staging

QAPI patches patches for 2021-05-20

# gpg: Signature made Thu 20 May 2021 16:10:21 BST
# gpg:                using RSA key 354BC8B3D7EB2A6B68674E5F3870B400EB918653
# gpg:                issuer "armbru@redhat.com"
# gpg: Good signature from "Markus Armbruster <armbru@redhat.com>" [full]
# gpg:                 aka "Markus Armbruster <armbru@pond.sub.org>" [full]
# Primary key fingerprint: 354B C8B3 D7EB 2A6B 6867  4E5F 3870 B400 EB91 8653

* remotes/armbru/tags/pull-qapi-2021-05-20:
  qapi/parser: add docstrings
  qapi/parser: allow 'ch' variable name
  qapi/parser: Remove superfluous list comprehension
  qapi/parser: add type hint annotations
  qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard
  qapi/parser: Fix token membership tests when token can be None
  qapi: add must_match helper
  qapi/parser: Use @staticmethod where appropriate
  qapi/parser: assert object keys are strings
  qapi/parser: enforce all top-level expressions must be dict in _parse()
  qapi/parser: Assert lexer value is a string
  qapi/parser: factor parsing routine into method
  qapi/source: Remove line number from QAPISourceInfo initializer
  qapi: Add test for nonexistent schema file
  qapi/parser: Don't try to handle file errors

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

scripts/qapi/common.py

@@ -12,7 +12,7 @@
 # See the COPYING file in the top-level directory.
 
 import re
-from typing import Optional, Sequence
+from typing import Match, Optional, Sequence
 
 
 #: Magic string that gets removed along with all space to its right.
@@ -210,3 +210,9 @@ def gen_endif(ifcond: Sequence[str]) -> str:
 #endif /* %(cond)s */
 ''', cond=ifc)
     return ret
+
+
+def must_match(pattern: str, string: str) -> Match[str]:
+    match = re.match(pattern, string)
+    assert match is not None
+    return match

scripts/qapi/main.py

@@ -8,11 +8,11 @@ This is the main entry point for generating C code from the QAPI schema.
 """
 
 import argparse
-import re
 import sys
 from typing import Optional
 
 from .commands import gen_commands
+from .common import must_match
 from .error import QAPIError
 from .events import gen_events
 from .introspect import gen_introspect
@@ -22,9 +22,7 @@ from .visit import gen_visit
 
 
 def invalid_prefix_char(prefix: str) -> Optional[str]:
-    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
+    match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
-    # match cannot be None, but mypy cannot infer that.
-    assert match is not None
     if match.end() != len(prefix):
         return prefix[match.end()]
     return None

scripts/qapi/parser.py

@@ -17,14 +17,26 @@
 from collections import OrderedDict
 import os
 import re
+from typing import (
+    Dict,
+    List,
+    Optional,
+    Set,
+    Union,
+)
 
+from .common import must_match
 from .error import QAPISemError, QAPISourceError
 from .source import QAPISourceInfo
 
 
+# Return value alias for get_expr().
+_ExprValue = Union[List[object], Dict[str, object], str, bool]
+
+
 class QAPIParseError(QAPISourceError):
     """Error class for all QAPI schema parsing errors."""
-    def __init__(self, parser, msg):
+    def __init__(self, parser: 'QAPISchemaParser', msg: str):
         col = 1
         for ch in parser.src[parser.line_pos:parser.pos]:
             if ch == '\t':
@@ -35,31 +47,69 @@ class QAPIParseError(QAPISourceError):
 
 
 class QAPISchemaParser:
+    """
+    Parse QAPI schema source.
 
-    def __init__(self, fname, previously_included=None, incl_info=None):
+    Parse a JSON-esque schema file and process directives.  See
-        previously_included = previously_included or set()
+    qapi-code-gen.txt section "Schema Syntax" for the exact syntax.
-        previously_included.add(os.path.abspath(fname))
+    Grammatical validation is handled later by `expr.check_exprs()`.
 
-        try:
+    :param fname: Source file name.
-            fp = open(fname, 'r', encoding='utf-8')
+    :param previously_included:
-            self.src = fp.read()
+        The absolute names of previously included source files,
-        except IOError as e:
+        if being invoked from another parser.
-            raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
+    :param incl_info:
-                               "can't read %s file '%s': %s"
+       `QAPISourceInfo` belonging to the parent module.
-                               % ("include" if incl_info else "schema",
+       ``None`` implies this is the root module.
-                                  fname,
-                                  e.strerror))
 
-        if self.src == '' or self.src[-1] != '\n':
+    :ivar exprs: Resulting parsed expressions.
-            self.src += '\n'
+    :ivar docs: Resulting parsed documentation blocks.
+
+    :raise OSError: For problems reading the root schema document.
+    :raise QAPIError: For errors in the schema source.
+    """
+    def __init__(self,
+                 fname: str,
+                 previously_included: Optional[Set[str]] = None,
+                 incl_info: Optional[QAPISourceInfo] = None):
+        self._fname = fname
+        self._included = previously_included or set()
+        self._included.add(os.path.abspath(self._fname))
+        self.src = ''
+
+        # Lexer state (see `accept` for details):
+        self.info = QAPISourceInfo(self._fname, incl_info)
+        self.tok: Union[None, str] = None
+        self.pos = 0
         self.cursor = 0
-        self.info = QAPISourceInfo(fname, 1, incl_info)
+        self.val: Optional[Union[bool, str]] = None
         self.line_pos = 0
-        self.exprs = []
+
-        self.docs = []
+        # Parser output:
-        self.accept()
+        self.exprs: List[Dict[str, object]] = []
+        self.docs: List[QAPIDoc] = []
+
+        # Showtime!
+        self._parse()
+
+    def _parse(self) -> None:
+        """
+        Parse the QAPI schema document.
+
+        :return: None.  Results are stored in ``.exprs`` and ``.docs``.
+        """
         cur_doc = None
 
+        # May raise OSError; allow the caller to handle it.
+        with open(self._fname, 'r', encoding='utf-8') as fp:
+            self.src = fp.read()
+        if self.src == '' or self.src[-1] != '\n':
+            self.src += '\n'
+
+        # Prime the lexer:
+        self.accept()
+
+        # Parse until done:
         while self.tok is not None:
             info = self.info
             if self.tok == '#':
@@ -68,7 +118,11 @@ class QAPISchemaParser:
                     self.docs.append(cur_doc)
                 continue
 
-            expr = self.get_expr(False)
+            expr = self.get_expr()
+            if not isinstance(expr, dict):
+                raise QAPISemError(
+                    info, "top-level expression must be an object")
+
             if 'include' in expr:
                 self.reject_expr_doc(cur_doc)
                 if len(expr) != 1:
@@ -77,12 +131,12 @@ class QAPISchemaParser:
                 if not isinstance(include, str):
                     raise QAPISemError(info,
                                        "value of 'include' must be a string")
-                incl_fname = os.path.join(os.path.dirname(fname),
+                incl_fname = os.path.join(os.path.dirname(self._fname),
                                           include)
                 self.exprs.append({'expr': {'include': incl_fname},
                                    'info': info})
                 exprs_include = self._include(include, info, incl_fname,
-                                              previously_included)
+                                              self._included)
                 if exprs_include:
                     self.exprs.extend(exprs_include.exprs)
                     self.docs.extend(exprs_include.docs)
@@ -109,17 +163,22 @@ class QAPISchemaParser:
         self.reject_expr_doc(cur_doc)
 
     @staticmethod
-    def reject_expr_doc(doc):
+    def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
         if doc and doc.symbol:
             raise QAPISemError(
                 doc.info,
                 "documentation for '%s' is not followed by the definition"
                 % doc.symbol)
 
-    def _include(self, include, info, incl_fname, previously_included):
+    @staticmethod
+    def _include(include: str,
+                 info: QAPISourceInfo,
+                 incl_fname: str,
+                 previously_included: Set[str]
+                 ) -> Optional['QAPISchemaParser']:
         incl_abs_fname = os.path.abspath(incl_fname)
         # catch inclusion cycle
-        inf = info
+        inf: Optional[QAPISourceInfo] = info
         while inf:
             if incl_abs_fname == os.path.abspath(inf.fname):
                 raise QAPISemError(info, "inclusion loop for %s" % include)
@@ -129,34 +188,86 @@ class QAPISchemaParser:
         if incl_abs_fname in previously_included:
             return None
 
-        return QAPISchemaParser(incl_fname, previously_included, info)
+        try:
-
+            return QAPISchemaParser(incl_fname, previously_included, info)
-    def _check_pragma_list_of_str(self, name, value, info):
+        except OSError as err:
-        if (not isinstance(value, list)
-                or any([not isinstance(elt, str) for elt in value])):
             raise QAPISemError(
                 info,
-                "pragma %s must be a list of strings" % name)
+                f"can't read include file '{incl_fname}': {err.strerror}"
+            ) from err
+
+    @staticmethod
+    def _pragma(name: str, value: object, info: QAPISourceInfo) -> None:
+
+        def check_list_str(name: str, value: object) -> List[str]:
+            if (not isinstance(value, list) or
+                    any(not isinstance(elt, str) for elt in value)):
+                raise QAPISemError(
+                    info,
+                    "pragma %s must be a list of strings" % name)
+            return value
+
+        pragma = info.pragma
 
-    def _pragma(self, name, value, info):
         if name == 'doc-required':
             if not isinstance(value, bool):
                 raise QAPISemError(info,
                                    "pragma 'doc-required' must be boolean")
-            info.pragma.doc_required = value
+            pragma.doc_required = value
         elif name == 'command-name-exceptions':
-            self._check_pragma_list_of_str(name, value, info)
+            pragma.command_name_exceptions = check_list_str(name, value)
-            info.pragma.command_name_exceptions = value
         elif name == 'command-returns-exceptions':
-            self._check_pragma_list_of_str(name, value, info)
+            pragma.command_returns_exceptions = check_list_str(name, value)
-            info.pragma.command_returns_exceptions = value
         elif name == 'member-name-exceptions':
-            self._check_pragma_list_of_str(name, value, info)
+            pragma.member_name_exceptions = check_list_str(name, value)
-            info.pragma.member_name_exceptions = value
         else:
             raise QAPISemError(info, "unknown pragma '%s'" % name)
 
-    def accept(self, skip_comment=True):
+    def accept(self, skip_comment: bool = True) -> None:
+        """
+        Read and store the next token.
+
+        :param skip_comment:
+            When false, return COMMENT tokens ("#").
+            This is used when reading documentation blocks.
+
+        :return:
+            None.  Several instance attributes are updated instead:
+
+            - ``.tok`` represents the token type.  See below for values.
+            - ``.info`` describes the token's source location.
+            - ``.val`` is the token's value, if any.  See below.
+            - ``.pos`` is the buffer index of the first character of
+              the token.
+
+        * Single-character tokens:
+
+            These are "{", "}", ":", ",", "[", and "]".
+            ``.tok`` holds the single character and ``.val`` is None.
+
+        * Multi-character tokens:
+
+          * COMMENT:
+
+            This token is not normally returned by the lexer, but it can
+            be when ``skip_comment`` is False.  ``.tok`` is "#", and
+            ``.val`` is a string including all chars until end-of-line,
+            including the "#" itself.
+
+          * STRING:
+
+            ``.tok`` is "'", the single quote.  ``.val`` contains the
+            string, excluding the surrounding quotes.
+
+          * TRUE and FALSE:
+
+            ``.tok`` is either "t" or "f", ``.val`` will be the
+            corresponding bool value.
+
+          * EOF:
+
+            ``.tok`` and ``.val`` will both be None at EOF.
+        """
         while True:
             self.tok = self.src[self.cursor]
             self.pos = self.cursor
@@ -216,12 +327,12 @@ class QAPISchemaParser:
             elif not self.tok.isspace():
                 # Show up to next structural, whitespace or quote
                 # character
-                match = re.match('[^[\\]{}:,\\s\'"]+',
+                match = must_match('[^[\\]{}:,\\s\'"]+',
-                                 self.src[self.cursor-1:])
+                                   self.src[self.cursor-1:])
                 raise QAPIParseError(self, "stray '%s'" % match.group(0))
 
-    def get_members(self):
+    def get_members(self) -> Dict[str, object]:
-        expr = OrderedDict()
+        expr: Dict[str, object] = OrderedDict()
         if self.tok == '}':
             self.accept()
             return expr
@@ -229,13 +340,15 @@ class QAPISchemaParser:
             raise QAPIParseError(self, "expected string or '}'")
         while True:
             key = self.val
+            assert isinstance(key, str)  # Guaranteed by tok == "'"
+
             self.accept()
             if self.tok != ':':
                 raise QAPIParseError(self, "expected ':'")
             self.accept()
             if key in expr:
                 raise QAPIParseError(self, "duplicate key '%s'" % key)
-            expr[key] = self.get_expr(True)
+            expr[key] = self.get_expr()
             if self.tok == '}':
                 self.accept()
                 return expr
@@ -245,16 +358,16 @@ class QAPISchemaParser:
             if self.tok != "'":
                 raise QAPIParseError(self, "expected string")
 
-    def get_values(self):
+    def get_values(self) -> List[object]:
-        expr = []
+        expr: List[object] = []
         if self.tok == ']':
             self.accept()
             return expr
-        if self.tok not in "{['tf":
+        if self.tok not in tuple("{['tf"):
             raise QAPIParseError(
                 self, "expected '{', '[', ']', string, or boolean")
         while True:
-            expr.append(self.get_expr(True))
+            expr.append(self.get_expr())
             if self.tok == ']':
                 self.accept()
                 return expr
@@ -262,16 +375,16 @@ class QAPISchemaParser:
                 raise QAPIParseError(self, "expected ',' or ']'")
             self.accept()
 
-    def get_expr(self, nested):
+    def get_expr(self) -> _ExprValue:
-        if self.tok != '{' and not nested:
+        expr: _ExprValue
-            raise QAPIParseError(self, "expected '{'")
         if self.tok == '{':
             self.accept()
             expr = self.get_members()
         elif self.tok == '[':
             self.accept()
             expr = self.get_values()
-        elif self.tok in "'tf":
+        elif self.tok in tuple("'tf"):
+            assert isinstance(self.val, (str, bool))
             expr = self.val
             self.accept()
         else:
@@ -279,7 +392,7 @@ class QAPISchemaParser:
                 self, "expected '{', '[', string, or boolean")
         return expr
 
-    def get_doc(self, info):
+    def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
         if self.val != '##':
             raise QAPIParseError(
                 self, "junk after '##' at start of documentation comment")
@@ -288,6 +401,7 @@ class QAPISchemaParser:
         cur_doc = QAPIDoc(self, info)
         self.accept(False)
         while self.tok == '#':
+            assert isinstance(self.val, str)
             if self.val.startswith('##'):
                 # End of doc comment
                 if self.val != '##':
@@ -346,7 +460,7 @@ class QAPIDoc:
             # Strip leading spaces corresponding to the expected indent level
             # Blank lines are always OK.
             if line:
-                indent = re.match(r'\s*', line).end()
+                indent = must_match(r'\s*', line).end()
                 if indent < self._indent:
                     raise QAPIParseError(
                         self._parser,
@@ -482,7 +596,7 @@ class QAPIDoc:
             # from line and replace it with spaces so that 'f' has the
             # same index as it did in the original line and can be
             # handled the same way we will handle following lines.
-            indent = re.match(r'@\S*:\s*', line).end()
+            indent = must_match(r'@\S*:\s*', line).end()
             line = line[indent:]
             if not line:
                 # Line was just the "@arg:" header; following lines
@@ -517,7 +631,7 @@ class QAPIDoc:
             # from line and replace it with spaces so that 'f' has the
             # same index as it did in the original line and can be
             # handled the same way we will handle following lines.
-            indent = re.match(r'@\S*:\s*', line).end()
+            indent = must_match(r'@\S*:\s*', line).end()
             line = line[indent:]
             if not line:
                 # Line was just the "@arg:" header; following lines
@@ -563,7 +677,7 @@ class QAPIDoc:
             # from line and replace it with spaces so that 'f' has the
             # same index as it did in the original line and can be
             # handled the same way we will handle following lines.
-            indent = re.match(r'\S*:\s*', line).end()
+            indent = must_match(r'\S*:\s*', line).end()
             line = line[indent:]
             if not line:
                 # Line was just the "Section:" header; following lines

scripts/qapi/pylintrc

@@ -43,6 +43,7 @@ good-names=i,
            _,
            fp,  # fp = open(...)
            fd,  # fd = os.open(...)
+           ch,
 
 [VARIABLES]
 

scripts/qapi/schema.py

@@ -20,7 +20,7 @@ import re
 from typing import Optional
 
 from .common import POINTER_SUFFIX, c_name
-from .error import QAPISemError, QAPISourceError
+from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
 from .parser import QAPISchemaParser
 
@@ -849,7 +849,14 @@ class QAPISchemaEvent(QAPISchemaEntity):
 class QAPISchema:
     def __init__(self, fname):
         self.fname = fname
-        parser = QAPISchemaParser(fname)
+
+        try:
+            parser = QAPISchemaParser(fname)
+        except OSError as err:
+            raise QAPIError(
+                f"can't read schema file '{fname}': {err.strerror}"
+            ) from err
+
         exprs = check_exprs(parser.exprs)
         self.docs = parser.docs
         self._entity_list = []

scripts/qapi/source.py

@@ -10,7 +10,6 @@
 # See the COPYING file in the top-level directory.
 
 import copy
-import sys
 from typing import List, Optional, TypeVar
 
 
@@ -32,10 +31,9 @@ class QAPISchemaPragma:
 class QAPISourceInfo:
     T = TypeVar('T', bound='QAPISourceInfo')
 
-    def __init__(self, fname: str, line: int,
+    def __init__(self, fname: str, parent: Optional['QAPISourceInfo']):
-                 parent: Optional['QAPISourceInfo']):
         self.fname = fname
-        self.line = line
+        self.line = 1
         self.parent = parent
         self.pragma: QAPISchemaPragma = (
             parent.pragma if parent else QAPISchemaPragma()
@@ -53,12 +51,7 @@ class QAPISourceInfo:
         return info
 
     def loc(self) -> str:
-        if self.fname is None:
+        return f"{self.fname}:{self.line}"
-            return sys.argv[0]
-        ret = self.fname
-        if self.line is not None:
-            ret += ':%d' % self.line
-        return ret
 
     def in_defn(self) -> str:
         if self.defn_name:

tests/qapi-schema/meson.build

@@ -134,9 +134,11 @@ schemas = [
   'indented-expr.json',
   'leading-comma-list.json',
   'leading-comma-object.json',
+  'missing-array-rsqb.json',
   'missing-colon.json',
   'missing-comma-list.json',
   'missing-comma-object.json',
+  'missing-object-member-element.json',
   'missing-type.json',
   'nested-struct-data.json',
   'nested-struct-data-invalid-dict.json',
@@ -199,11 +201,16 @@ schemas = [
   'unknown-escape.json',
   'unknown-expr-key.json',
 ]
+schemas = files(schemas)
+
+# Intentionally missing schema file test -- not passed through files():
+schemas += [meson.current_source_dir() / 'missing-schema.json']
 
 # Because people may want to use test-qapi.py from the command line, we
 # are not using the "#! /usr/bin/env python3" trick here.  See
 # docs/devel/build-system.txt
-test('QAPI schema regression tests', python, args: files('test-qapi.py', schemas),
+test('QAPI schema regression tests', python,
+     args: files('test-qapi.py') + schemas,
      env: test_env, suite: ['qapi-schema', 'qapi-frontend'])
 
 diff = find_program('diff')

tests/qapi-schema/missing-array-rsqb.err

@@ -0,0 +1 @@
+missing-array-rsqb.json:1:44: expected '{', '[', string, or boolean

tests/qapi-schema/missing-array-rsqb.json

@@ -0,0 +1 @@
+['Daisy,', 'Daisy,', 'Give me your answer',

tests/qapi-schema/missing-object-member-element.err

@@ -0,0 +1 @@
+missing-object-member-element.json:1:8: expected '{', '[', string, or boolean

tests/qapi-schema/missing-object-member-element.json

@@ -0,0 +1 @@
+{'key':

tests/qapi-schema/missing-schema.err

@@ -0,0 +1 @@
+can't read schema file 'missing-schema.json': No such file or directory

tests/qapi-schema/non-objects.err

@@ -1 +1 @@
-non-objects.json:1:1: expected '{'
+non-objects.json:1: top-level expression must be an object

tests/qapi-schema/quoted-structural-chars.err

@@ -1 +1 @@
-quoted-structural-chars.json:1:1: expected '{'
+quoted-structural-chars.json:1: top-level expression must be an object

tests/qapi-schema/test-qapi.py

@@ -128,9 +128,6 @@ def test_and_diff(test_name, dir_name, update):
     try:
         test_frontend(os.path.join(dir_name, test_name + '.json'))
     except QAPIError as err:
-        if err.info.fname is None:
-            print("%s" % err, file=sys.stderr)
-            return 2
         errstr = str(err) + '\n'
         if dir_name:
             errstr = errstr.replace(dir_name + '/', '')