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:
>> >> >> 
>> >> >> > * 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.
>
> Hmm the visit_end_* are interesting; I guess we have to be careful
> of those, unless that is you could make the visit_end_struct(v, NULL)
> to fail nicely in that case.

Unfortunately, making functions fail "nicely" on programming errors
creates a new burden for users that do not want to recover from
programming errors (because it's *unsafe*): now they have to somehow
find out whether the error they got is a programming error.  That's not
even possible right now, but we could add a "this is a programming
error" predicate to the error type.  Anyway, the error boilerplate would
then explode, and the error handling bugs would multiply.

You can't make the same API serve radically different requirements
equally well.  "Fail as cleanly as possible even on violations of
preconditions and invariants, and do your best to survive regardless of
how I handle them" is radically different from "rely on preconditions
and invariants, but do check them to catch bugs".  An interface doing
the former is bound to have weaker postconditions.  It's also harder to
implement, and harder to use.

>> 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.
>
> How would we deal with all the visit_end_* - if we've decided
> there's an error are we required to call all the end's before we
> just free the visitor or something like that?

After this series, the visitor contract guarantees (in writing) that you
can safely destroy or reset the visitor at any time.

Existing users call the matching visit_end_FOO() instead, because that's
easy enough for them.  But if you find yourself in a situation where you
don't want to, go ahead and destroy the visitor without further ado.

>> >> 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.
>
> OK, lets keep our own writer; if we can't have more control over visitors
> failure paths, we'll have to.

All right, I'll cook up a patch.