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

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> "Dr. David Alan Gilbert" <address@hidden> writes:
> 
> > * Markus Armbruster (address@hidden) wrote:
> >> "Dr. David Alan Gilbert" <address@hidden> 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 / address@hidden / Manchester, UK