From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>,
Cleber Rosa <crosa@redhat.com>
Subject: Re: [PATCH 01/22] qapi/parser: Don't try to handle file errors
Date: Wed, 28 Apr 2021 07:48:37 +0200 [thread overview]
Message-ID: <87bl9zq7re.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <1df92443-b8e0-a6e4-e0af-fb562a3579f1@redhat.com> (John Snow's message of "Tue, 27 Apr 2021 13:58:39 -0400")
John Snow <jsnow@redhat.com> writes:
> On 4/27/21 9:47 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 4/23/21 11:46 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> 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.
>>>>
>>>> I'm not sure I get what you're trying to say here.
>>>>
>>>
>>> I mean: this __init__ method does not *know* who is calling it or why.
>>> Of course, *we* do, because the code base is finite and nobody else but
>>> us is calling into it.
>>>
>>> I mean to point out that the initializer has to do extra work (Just a
>>> little) to determine what the calling context is and raise an error
>>> accordingly.
>>>
>>> Example: If we have a parent info context, we raise an error in the
>>> context of the caller. If we don't, we have to create a new presumed
>>> context (using the weird None SourceInfo object).
>>
>> I guess you mean
>>
>> raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
>>
>> I can't see other instances of messing with context.
>>
>
> Yes, and the string construction that follows, too. It's all about
> trying to understand who our caller is and raising an error appropriate
> for them on their behalf.
I guess you can view it that way. I never did. My thinking was
@fname either comes from a schema file (@incl_info is not None) or
somewhere else. If schema file, make the exception's __str__()
start with "SCHEMA-FILE:LINE: ", because that's how compilers report
errors in source files. Else, make it start with just "PROGNAME: ",
because that's how compilers report errors unrelated to source
files.
This assumes "incl_info is None implies unrelated to source file". I
think that's fair. I don't think it rises to the level of
"understanding who our caller is".
>>> So I just mean to say:
>>>
>>> "Let the caller, who unambiguously always has the exactly correct
>>> context worry about what the error message ought to be."
[...]
>>>> 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.)
>>
>> Good to know.
>>
>> However, I was talking about the code covered by try ... except OSError
>> (or IOError, or whatever). Before the patch, it's just open() and
>> .read(). Afterwards it's all of .__init__().
>>
>
> Apologies, I misread.
>
>> Could anything else in .__init__() possibly raise OSError? Probably
>> not, but it's not trivially obvious. Which makes me go "hmm."
>>
>> "Hmm" isn't "no", it's just "hmm".
>>
>
> Yeah, it is rather broad. That is one of the perils of doing *so much*
> at init() time, in my opinion.
It's not ideal, but then having to write something like
parser = QAPISchemaParser(fname).parse()
or
parser = QAPISchemaParser().parse(fname)
instead of just
parser = QAPISchemaParser(fname)
would be less than ideal, too.
> We don't make any other syscalls in the parser though, so it should be
> fine. The docstring patch later documents the errors we expect to see
> here, so it becomes a visible part of the interface.
[...]
next prev parent reply other threads:[~2021-04-28 5:49 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-22 3:06 [PATCH 00/22] qapi: static typing conversion, pt5a John Snow
2021-04-22 3:06 ` [PATCH 01/22] qapi/parser: Don't try to handle file errors John Snow
2021-04-23 15:46 ` Markus Armbruster
2021-04-23 19:20 ` John Snow
2021-04-27 13:47 ` Markus Armbruster
2021-04-27 17:58 ` John Snow
2021-04-28 5:48 ` Markus Armbruster [this message]
2021-04-22 3:07 ` [PATCH 02/22] qapi/source: [RFC] add "with_column" contextmanager John Snow
2021-04-27 9:33 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 03/22] qapi/source: Remove line number from QAPISourceInfo initializer John Snow
2021-04-24 6:38 ` Markus Armbruster
2021-04-26 17:39 ` John Snow
2021-04-26 23:14 ` John Snow
2021-04-27 6:07 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 04/22] qapi/parser: factor parsing routine into method John Snow
2021-04-22 3:07 ` [PATCH 05/22] qapi/parser: Assert lexer value is a string John Snow
2021-04-24 8:33 ` Markus Armbruster
2021-04-26 17:43 ` John Snow
2021-04-27 12:30 ` Markus Armbruster
2021-04-27 13:58 ` John Snow
2021-04-22 3:07 ` [PATCH 06/22] qapi/parser: assert get_expr returns object in outer loop John Snow
2021-04-25 7:23 ` Markus Armbruster
2021-04-27 15:03 ` John Snow
2021-04-22 3:07 ` [PATCH 07/22] qapi/parser: assert object keys are strings John Snow
2021-04-25 7:27 ` Markus Armbruster
2021-04-26 17:46 ` John Snow
2021-04-27 6:13 ` Markus Armbruster
2021-04-27 14:15 ` John Snow
2021-04-22 3:07 ` [PATCH 08/22] qapi/parser: Use @staticmethod where appropriate John Snow
2021-04-22 3:07 ` [PATCH 09/22] qapi: add match_nofail helper John Snow
2021-04-25 7:54 ` Markus Armbruster
2021-04-26 17:48 ` John Snow
2021-04-22 3:07 ` [PATCH 10/22] qapi/parser: Fix typing of token membership tests John Snow
2021-04-25 7:59 ` Markus Armbruster
2021-04-26 17:51 ` John Snow
2021-04-27 7:00 ` Markus Armbruster
2021-05-04 1:01 ` John Snow
2021-05-05 6:29 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 11/22] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard John Snow
2021-04-25 12:32 ` Markus Armbruster
2021-04-26 23:48 ` John Snow
2021-04-27 7:15 ` Markus Armbruster
2021-05-05 19:09 ` John Snow
2021-04-22 3:07 ` [PATCH 12/22] qapi/parser: add type hint annotations John Snow
2021-04-25 12:34 ` Markus Armbruster
2021-04-26 18:00 ` John Snow
2021-04-27 8:21 ` Markus Armbruster
2021-04-26 23:55 ` John Snow
2021-04-27 8:43 ` Markus Armbruster
2021-05-06 1:49 ` John Snow
2021-05-06 1:27 ` John Snow
2021-04-22 3:07 ` [PATCH 13/22] qapi/parser: [RFC] overload the return type of get_expr John Snow
2021-04-22 3:07 ` [PATCH 14/22] qapi/parser: Remove superfluous list constructor John Snow
2021-04-22 3:07 ` [PATCH 15/22] qapi/parser: allow 'ch' variable name John Snow
2021-04-22 3:07 ` [PATCH 16/22] qapi/parser: add docstrings John Snow
2021-04-25 13:27 ` Markus Armbruster
2021-04-26 18:26 ` John Snow
2021-04-27 9:03 ` Markus Armbruster
2021-05-06 2:08 ` John Snow
2021-05-07 1:34 ` John Snow
2021-05-07 8:25 ` Markus Armbruster
2021-04-22 3:07 ` [PATCH 17/22] CHECKPOINT John Snow
2021-04-22 3:07 ` [PATCH 18/22] qapi: [WIP] Rip QAPIDoc out of parser.py John Snow
2021-04-22 3:07 ` [PATCH 19/22] qapi: [WIP] Add type ignores for qapidoc.py John Snow
2021-04-22 3:07 ` [PATCH 20/22] qapi: [WIP] Import QAPIDoc from qapidoc Signed-off-by: John Snow <jsnow@redhat.com> John Snow
2021-04-22 3:07 ` [PATCH 21/22] qapi: [WIP] Add QAPIDocError John Snow
2021-04-22 3:07 ` [PATCH 22/22] qapi: [WIP] Enable linters on parser.py John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bl9zq7re.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=michael.roth@amd.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).