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:

> "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 / dgilbert@redhat.com / Manchester, UK