Before commit c615550df3 "qapi: Improve source file read error handling"
(v4.2.0), it was differently bad (uncaught exception).
Commit c615550df3 explains why the funny QAPISourceInfo exists:
Reporting open or read failure for the main schema file needs a
QAPISourceInfo representing "no source". Make QAPISourceInfo cope
with fname=None.
I am apparently not the first or the last person to dream of wanting a
QAPISourceInfo that represents "Actually, there's no source location!"
The commit turned QAPISourceInfo into the equivalent of a disjoint union
of
1. A position in a source file (.fname is a str)
2. "Not in any source file" (.fname is None)
This is somewhat similar to struct Location in C, which has
1. LOC_FILE: a position in a source file
2. LOC_CMDLINE: a range of command line arguments
3. LOC_NONE: no location information
Abstracting locations this way lets error_report() do the right thing
whether its complaining about the command line, a monitor command, or a
configuration file read with -readconfig.
Your patch demonstrates that qapi-gen has much less need for abstracting
sources: we use 2. "Not in any source file" only for reading the main
schema file.
Yes. I got the impression that you didn't want to pursue more abstract
QSI constructs based on earlier work, so going the other way and
*removing* them seemed like the faster way to achieve a clean type
system here.
In pursuing it, we find that QAPISourceInfo has a special accommodation
for when there's no filename.
Yes:
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
return ret
Meanwhile, we intend to type info.fname as
str; something we always have.
Do you mean "as non-optional str"?
Yeah. I typed it originally as `str`, but the analyzer missed that we
check the field to see if it's None, which is misleading.
To remove this, we need to not have a "fake" QAPISourceInfo object. We
We may well want to, but I doubt we *need* to. There are almost
certainly other ways to fix the bug. I don't see a need to explore
them, though.
Either we build out the fake QSI into a proper subtype, or we remove it
-- those are the two obvious options. Building it out is almost
certainly more work than this patch.
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".
Yes, encoding both "poisoned source info not to be used with actual
errors" and "'fake' source info not pointing to a source file" as None
would be a mistake.
:)
(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:
Missing "again" after "sensible" ;-P
okayokayokayfine
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.
"this error"? I understand what you mean now, but I'm not sure I will
in three months, when I won't have the context I have now.
Yep, OK.
# May raise OSError, allow the caller to handle it.
+ 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)
Before the patch, only IOError from open() and .read() get converted to
QAPISemError, and therefore caught by main().
The patch widen this to anywhere in QAPISchemaParser.__init__(). Hmm.
"Changed in version 3.3: EnvironmentError, IOError, WindowsError,
socket.error, select.error and mmap.error have been merged into OSError,
and the constructor may return a subclass."
>>> OSError == IOError
True
(No, I didn't know this before I wrote it. I just intentionally wanted
to catch everything that open() might return, which I had simply assumed
was not fully captured by IOError. Better to leave it as OSError now to
avoid misleading anyone into thinking it's more narrow than it really is.)