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

[Markus Armbruster]

"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!

>> >> 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.

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?