[PATCH 01/22] qapi/parser: Don't try to handle file errors

[John Snow]

The short-ish version of what motivates this patch is:

- The parser initializer does not possess adequate context to write a
  good error message -- It tries to determine the caller's semantic
  context.
- We don't want to allow QAPISourceInfo(None, None, None) to exist.
- Errors made using such an object are currently incorrect.
- It's not technically a semantic error if we cannot open the schema
- There are various typing constraints that make mixing these two cases
  undesirable for a single special case.
- The current open block in parser's initializer will leak file
  pointers, because it isn't using a with statement.


Here's the details in why this got written the way it did, and why a few
disparate issues are rolled into one commit. (They're hard to fix
separately without writing really weird stuff that'd be harder to
review.)

The error message string here is incorrect:

> python3 qapi-gen.py 'fake.json'
qapi-gen.py: qapi-gen.py: can't read schema file 'fake.json': No such file or 
directory

In pursuing it, we find that QAPISourceInfo has a special accommodation
for when there's no filename. Meanwhile, we intend to type info.fname as
str; something we always have.

To remove this, we need to not have a "fake" QAPISourceInfo object. We
also don't want to explicitly begin accommodating QAPISourceInfo being
None, because we actually want to eventually prove that this can never
happen -- We don't want to confuse "The file isn't open yet" with "This
error stems from a definition that wasn't defined in any file".

(An earlier series tried to create an official dummy object, but it was
tough to prove in review that it worked correctly without creating new
regressions. This patch avoids trying to re-litigate that discussion.

We would like to first prove that we never raise QAPISemError for any
built-in object before we relent and add "special" info objects. We
aren't ready to do that yet, so crashing is preferred.)

So, how to solve this mess?

Here's one way: Don't try to handle errors at a level with "mixed"
semantic levels; i.e. don't try to handle inclusion errors (should
report a source line where the include was triggered) with command line
errors (where we specified a file we couldn't read).

Simply remove the error handling from the initializer of the
parser. Pythonic! Now it's the caller's job to figure out what to do
about it. Handle the error in QAPISchemaParser._include() instead, where
we do have the correct semantic context to not need to play games with
the error message generation.

Next, to re-gain a nice error at the top level, add a new try/except
into qapi/main.generate(). Now the error looks sensible:

> python3 qapi-gen.py 'fake.json'
qapi-gen.py: can't read schema file 'fake.json': No such file or directory

Lastly, with this usage gone, we can remove the special type violation
from QAPISourceInfo, and all is well with the world.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/main.py   |  8 +++++++-
 scripts/qapi/parser.py | 18 +++++++++---------
 scripts/qapi/source.py |  3 ---
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 703e7ed1ed5..70f8aa86f37 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -48,7 +48,13 @@ def generate(schema_file: str,
     """
     assert invalid_prefix_char(prefix) is None
 
-    schema = QAPISchema(schema_file)
+    try:
+        schema = QAPISchema(schema_file)
+    except OSError as err:
+        raise QAPIError(
+            f"can't read schema file '{schema_file}': {err.strerror}"
+        ) from err
+
     gen_types(schema, output_dir, prefix, builtins)
     gen_visit(schema, output_dir, prefix, builtins)
     gen_commands(schema, output_dir, prefix)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ca5e8e18e00..b378fa33807 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -40,15 +40,9 @@ def __init__(self, fname, previously_included=None, 
incl_info=None):
         previously_included = previously_included or set()
         previously_included.add(os.path.abspath(fname))
 
-        try:
-            fp = open(fname, 'r', encoding='utf-8')
+        # Allow the caller to catch this error.
+        with open(fname, 'r', encoding='utf-8') as fp:
             self.src = fp.read()
-        except IOError as e:
-            raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
-                               "can't read %s file '%s': %s"
-                               % ("include" if incl_info else "schema",
-                                  fname,
-                                  e.strerror))
 
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
@@ -129,7 +123,13 @@ def _include(self, include, info, incl_fname, 
previously_included):
         if incl_abs_fname in previously_included:
             return None
 
-        return QAPISchemaParser(incl_fname, previously_included, info)
+        try:
+            return QAPISchemaParser(incl_fname, previously_included, info)
+        except OSError as err:
+            raise QAPISemError(
+                info,
+                f"can't read include file '{incl_fname}': {err.strerror}"
+            ) from err
 
     def _check_pragma_list_of_str(self, name, value, info):
         if (not isinstance(value, list)
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 03b6ede0828..1ade864d7b9 100644
--- a/scripts/qapi/source.py
+++ b/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
 
 
@@ -53,8 +52,6 @@ def next_line(self: T) -> T:
         return info
 
     def loc(self) -> str:
-        if self.fname is None:
-            return sys.argv[0]
         ret = self.fname
         if self.line is not None:
             ret += ':%d' % self.line
-- 
2.30.2