* 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.
> >> >> Complete list of conditions where the JSON output visitor sets an error:
> >> >>
> >> >> * Conditions where the visitor core sets an error:
> >> >>
> >> >> - visit_type_uintN() when one of the visit_type_uint{8,16,32}() passes
> >> >> a value out of bounds. This is a serious programming error in
> >> >> qapi-visit-core.c. We're almost certainly screwed, and attempting
> >> >> to continue is unsafe.
> >> >>
> >> >> - visit_type_int(): likewise.
> >> >>
> >> >> - output_type_enum() when the numeric value is out of bounds. This is
> >> >> either a serious programming error in qapi-visit-core.c, or
> >> >> corrupted state. Either way, we're almost certainly screwed, and
> >> >> attempting to continue is unsafe.
> >> >>
> >> >> - input_type_enum() when the string value is unknown. This is either
> >> >> a serious programming error in qapi-visit-core.c, or bad input.
> >> >> However, the JSON output visitor isn't supposed to ever call
> >> >> input_type_enum(), so it's the former. Once again, we're almost
> >> >> certainly screwed, and attempting to continue is unsafe.
> >> >>
> >> >> * Conditions where the JSON output visitor itself sets an error:
> >> >>
> >> >> - None.
> >> >>
> >> >> Do you still object to &error_abort?
> >> >
> >> > So at the very least it should be commented as to why it can't happen.
> >> > My worry about it is that you've got a fairly long comment about why
> >> > it can't happen, and I worry that in 6 months someone adds a feature
> >> > to either the visitors or the migration code that means there's now
> >> > a case where it can happen.
> >>
> >> Here's why I don't think new failure modes are likely.
> >>
> >> What does this helper module do, and how could it possibly fail? By
> >> "possibly", I mean any conceivable reasonable implementation, not just
> >> the two we have (this patch gets rid of one).
> >>
> >> This helper module builds JSON text and returns it as a string. Its
> >> interface mirrors JSON abstract syntax: start object, end object, start
> >> array, end array, string, ... Additionally, initialize, finalize, get
> >> the result as a string.
> >>
> >> Conceivable failure modes:
> >>
> >> * Out of memory. We die, like we generally do for smallish allocations.
> >>
> >> * Data not representable in JSON. This is basically non-finite numbers,
> >> and we already chose to extend JSON instead of making this an error.
> >> Such a decision will not be revised without a thorough analysis of
> >> impact on existing users.
> >>
> >> * Interface misused, e.g. invalid nesting. Clearly a programming error.
> >> We can either silently produce garbage output, fail, or die. Before
> >> the patch: garbage output. After the patch: die by assertion failure
> >> (*not* via &error_abort).
> >>
> >> * Anything else?
> >>
> >> "Not via &error_abort" leads me to another point. The &error_abort are
> >> the assertions you can see in the patch. The ones you can't see are in
> >> the visitor core and the JSON output visitor. They're all about misuse
> >> of the interface.
> >>
> >> The old code is different: it doesn't detect misuse, and produces
> >> invalid JSON instead. "Never check for an error you don't know how to
> >> handle."
> >>
> >> With the new code, misuse should be caught in general migration testing,
> >> "make check" if it's any good.
> >>
> >> With the old code, it could more easily escape testing, because you have
> >> to parse the resulting JSON to detect it.
> >
> > And what happens to the users VM if that JSON is invalid? *nothing*
> > The user doesn't see any problem at all; no corruption, no crash, nothing.
> > That's what I like users to see.
>
> This assumes that the root cause of the assertion failure has no further
> ill effects. I call that assumption bold. But to each his own.
The whole JSON use in migration is just for debug/parsing in external tools -
even if it's complete rubbish it doesn't affect the VM, which is why I don't
want an error producing it to kill the VM.
> 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.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
