* 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.
>
> >> 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.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
