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:
> 
> > * 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,

Hang on though - this takes a Visitor* - that could be any visitor and that
could 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.

For an outgoing migration we must never kill the source unless we think the
data structures the source is using are itself corrupt.
We get programming errors both in our migration code and the migration
structures on devices.
If our migration code is broken/failing an invariant that still doesn't mean
you should kill the source - it should kill the migration only.

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

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK