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

[Markus Armbruster]

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.

[...]