Re: [PATCH 0/5] qobject: switch JSON parser to push

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il ven 30 gen 2026, 14:00 Markus Armbruster <armbru@redhat.com> ha scritto:
>
>> > Another benefit is that QEMU can report the first parsing error
>> > immediately, without waiting for delimiters to be balanced.
>>
>> Sounds promising!  Let's see...
>>
>> Before the series:
>>
>>      $ socat "READLINE,prompt=QMP> " UNIX-CONNECT:$HOME/work/images/test-qmp
>>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 10}, "package": "v10.2.0-567-gfb6b66de43-dirty"}, "capabilities": ["oob"]}}
>>     QMP> [{"a"]
>>
>> Parse error not diagnosed right away, but ...
>>
>>     QMP> }
>>     {"error": {"class": "GenericError", "desc": "JSON parse error, missing : in object pair"}}
>>
>> .... only when the streamer decides the expression is complete.
>>
>> After the series:
>>
>>     QMP> [{"a"]
>>     {"error": {"class": "GenericError", "desc": "JSON parse error at line 1, column 6, expecting ':'"}}
>>
>> Cool!  However, if I do it again, things fall apart:
>>
>>     QMP> [{"a"
>>     QMP> }
>>     QMP> }
>>     QMP> }
>>     QMP> ]
>>     QMP> ]
>>     {"error": {"class": "GenericError", "desc": "JSON parse error at line 7, column 1, expecting value"}}
>>
>> Parse error recovery not quite right?
>
> Well, if you read the above very carefully :) the error is *reported*
> immediately, but recovery still waits for delimiters to be balanced.

Yes, but the recovery regressed a bit.

Having read the entire series now, I wonder whether it's related to
capping @brace_count and @bracket_count at zero [PATCH 3].

My example input [{"a"] puts the parser in error recovery state with
brace_count == 1, bracket_count == 0.  Your parser will silently throw
away further input until brace_count drops to zero.

The old parser additionally snaps out of this unresponsive state when
bracked_count goes negative.

> In testing, when I got an error I just typed a long enough variant on
> "]}]}]}" and that is enough to recover—just like in the old parser.

With the old parser, I can park my finger on ']' or '}' for a moment,
hit return, and be good.  With the new one, I have to park on both.

More seriously, the new parser seems to break a recovery technique
documented in docs/interop/qmp-spec.rst:

    Forcing the JSON parser into known-good state
    ---------------------------------------------

    Incomplete or invalid input can leave the server's JSON parser in a
    state where it can't parse additional commands.  To get it back into
    known-good state, the client should provoke a lexical error.

    The cleanest way to do that is sending an ASCII control character
    other than ``\t`` (horizontal tab), ``\r`` (carriage return), or
    ``\n`` (new line).

Works with old parser:

    QMP> [{"a"]
    QMP> ^L
    {"error": {"class": "GenericError", "desc": "JSON parse error, stray '\f'"}}
    QMP> {}
    {"error": {"class": "GenericError", "desc": "QMP input lacks member 'execute'"}}

Note: the ^L is a formfeed character.

New parser:

    QMP> [{"a"]
    {"error": {"class": "GenericError", "desc": "JSON parse error at line 1, column 6, expecting ':'"}}
    QMP> ^L
    QMP> {}

Good: the parse error is reported immediately.

Bad: the formfeed no longer forces the parser into known-good state.
This needs fixing.

>                                                                     Still
> the difference in error reporting matters, because it gives feedback that
> is immediately useful, rather than possibly delayed forever.

Reporting parse errors immediately is such a lovely quality of life
improvement.  May I have that without regressing error recovery?

> The policy is easy to change, either in v2 or in subsequent work, because
> recovery is layered on top of json-parser and its code is nothing more than
> "if you believe it's a good time to recover, reset the parser".
>
> Paolo

>> > On top of the benefits intrinsic in the push architecture, it so happens
>> > that it's really easy to add a location to JSON parsing errors now, so
>> > do that as well.
>> >
>> > Paolo
>>
>>