Re: [Qemu-devel] [PATCH v3 10/18] vmstate: Use new JSON output visitor

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) wrote:
>> "Dr. David Alan Gilbert" <address@hidden> writes:
>> 
>> > * Markus Armbruster (address@hidden) wrote:
>> >> "Dr. David Alan Gilbert" <address@hidden> writes:
>> >> 
>> >> > * Markus Armbruster (address@hidden) wrote:
>> >> >> "Dr. David Alan Gilbert" <address@hidden> writes:
>> >> >
>> >> >> "git-grep assert migration" suggests you do kill the source on certain
>> >> >> programming errors.
>> >> >
>> >> > I'm just trying hard to reduce them; I know I'm not there, but I'd 
>> >> > rather
>> >> > we didn't have any - especially on the source side.
>> >> >
>> >> >> I reiterate my point that fancy, untestable error recovery is unlikely
>> >> >> to actually recover.  "Fancy" can work, "untestable" might work (but
>> >> >> color me skeptic), but once you got both, you're a dead man walking.
>> >> >
>> >> > Then we should make the error recovery paths easy; at the moment visitor
>> >> > error paths are just too painful.
>> >> 
>> >> I've never seen error handling in C that wasn't painful and still
>> >> correct.  Surprise me!
>> >
>> > The thing that makes it hard for the visitor code is the need to check
>> > it after every call and the check is complicated.
>> 
>> Having to check every call is certainly painful, but there's no general
>> and safe way around it.  Accumulating errors that need to be checked
>> only at the end of a job can be less painful, but then the job's code
>> needs to be very carefully written to be safe even in presence of
>> errors.  Most code isn't, and some code can't.
>
> Yes; output visitors would seem to be the easiest case though?

Here's the example from visitor.h at the end of this series (with a
small mistake corrected):

    Visitor *v;
    Error *err = NULL;
    int value;

    v = ...obtain visitor...
    visit_start_struct(v, NULL, NULL, 0, &err);
    if (err) {
        goto out;
    }
    visit_start_list(v, "list", NULL, 0, &err);
    if (err) {
        goto outobj;
    }
    value = 1;
    visit_type_int(v, NULL, &value, &err);
    if (err) {
        goto outlist;
    }
    value = 2;
    visit_type_int(v, NULL, &value, &err);
    if (err) {
        goto outlist;
    }
   outlist:
    visit_end_list(v, NULL);
    if (!err) {
        visit_check_struct(v, &err);
    }
   outobj:
    visit_end_struct(v, NULL);
   out:
    error_propagate(errp, err);
    ...clean up v...

With accumulating Errors, we could elide some but not all error checks.
In particular, the ones after visit_start_FOO() are still required,
because visit_end_FOO() may only be called after visit_start_FOO()
succeeded.

If we did anything interesting in addition to calling visitors, we'd
have to additionally consider whether doing it is safe after errors.

Accumulating errors *can* make the code easier on the eyes, but they
also make it easy to screw up behavior after error.

>> The check for failure is simple, but annoyingly verbose when the
>> function's return value is useless:
>> 
>>     Error *err = NULL;
>>     foo(..., &err);
>>     if (err) {
>>         ...
>>     }
>> 
>> I'm playing with a update to conventions and usage to permit
>> 
>>     if (!foo(..., &err)) {
>>         ...
>>     }
>
> If that became;
>       if (!foo(..., &err) ||
>           !foo(..., &err) ||
>           !foo(..., &err)) {
>           ...
>       }
>
> That would be both readable and not verbose.

Yes, that could be done then.

>> Just as simple, but more readable.
>> 
>> [...]
>> >> I figure we're unlikely to reach consensus on this, so I'd like to
>> >> propose we agree to disagree, and do the following:
>> >> 
>> >> * We shelve the de-duplication of JSON formatting (this patch)
>> >>   indefinitely.
>> >> 
>> >> * We move qjson.c to migration/, next to its only user, and add a
>> >>   comment explaining why it migration doesn't want to use general
>> >>   infrastructure here (JSON output visitor), but needs its own thing.
>> >>   This gets the file covered in MAINTAINERS, and will help prevent it
>> >>   growing additional users.
>> >> 
>> >> Deal?
>> >
>> > No, sorry; the JSON use in the migration is just a debug thing;
>> > we don't want to maintain a separate JSON instance for it.
>> 
>> Well, you already do, except in name.  Who else do you think is
>> maintaining qjson.[ch], created by migration people, for migration's
>> use?  Certainly not me.
>
> That came from migration? Really? I didn't think we used JSON at
> all until last year.

Commit 0457d07..b174257.

Migration is still the only user of this special JSON writer, and if you
ask me, it better remain the only one.

>> If you can't use the general JSON output code I maintain because of
>> special requirements, you get to continue maintaining your own.  All 109
>> SLOC of it.  All I'm asking is to make it official, and to deter
>> accidental use of migration's JSON writer instead of the general one.
>
> Yeh; I'd love to share the JSON code; just lets try and avoid anything that
> can kill the source, however broken the migration.

Visitors will abort when their preconditions or invariants are violated.
If that's not okay for migration, I'm afraid migration needs to continue
to roll its own JSON writer.  Visitors are pretty heavily used nowadays,
and we very much rely on these assertions to catch mistakes.