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

[Dr. David Alan Gilbert]

* 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.

> 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?

> >> 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.

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK