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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >> 
> >> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> 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?

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

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

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

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK