* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
>
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> >>
> >> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> >> >>
> >> >> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> >> >> "Dr. David Alan Gilbert" <dgilb...@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?
>
> 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 / dgilb...@redhat.com / Manchester, UK
