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

[Markus Armbruster <armbru@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Eric Blake (eblake@redhat.com) wrote:
>
>> -static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
>> +static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
>> +                                   Visitor *vmdesc)
>>  {
>>      int64_t old_offset, size;
>> +    const char *tmp;
>> 
>>      old_offset = qemu_ftell_fast(f);
>>      se->ops->save_state(f, se->opaque);
>>      size = qemu_ftell_fast(f) - old_offset;
>> 
>>      if (vmdesc) {
>> -        json_prop_int(vmdesc, "size", size);
>> -        json_start_array(vmdesc, "fields");
>> -        json_start_object(vmdesc, NULL);
>> -        json_prop_str(vmdesc, "name", "data");
>> -        json_prop_int(vmdesc, "size", size);
>> -        json_prop_str(vmdesc, "type", "buffer");
>> -        json_end_object(vmdesc);
>> -        json_end_array(vmdesc);
>> +        visit_type_int(vmdesc, "size", &size, &error_abort);
>> +        visit_start_list(vmdesc, "fields", NULL, 0, &error_abort);
>> +        visit_start_struct(vmdesc, NULL, NULL, 0, &error_abort);
>
> Please avoid error_abort in migration code, especially on the source side.
> You've got an apparently happily working VM, we must never kill it 
> while attempting migration.

These functions cannot fail, and &error_abort is a concise way to
express that.  It's the same as

            visit_type_int(vmdesc, "size", &size, &err);
            assert(!err);

An alternative would be ignoring errors:

            visit_type_int(vmdesc, "size", &size, NULL);

Ignoring violations of design invariants is hardly ever a good idea,
though.

Another alternative would be trying to recover from the violation, like
this:

            visit_type_int(vmdesc, "size", &size, &err);
            if (err) {
                report we're fscked...
                do whatever needs to be done to recover...
                goto out;
            }

Fancy untestable error paths are hardly ever good ideas, either.

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?