Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Juan Quintela <quintela@redhat.com>]

Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
>> * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
>>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:

> @Dave:
> There are a couple of questions I'm gonna have to ask/investigate should
> it be me doing the 'return value for pre_save' (also notes to myself):

As Dave said, just changing the prototype and fix callers will do a lot
of good.

> (keep the current qemu_file_set_error mechanism) or would you prefer
> things converted? IMHO having a single method would be cleaner, but I
> have not looked into this in great detail.

Error handling is a mess in qemu (in general), and worse in migration
(in particular).

History lesson (that is even older than my involvement with qemu)

It used to be that evrything was done with callbacks in the main loop.
So there were not an easy way to pass error messages.  Everything was
done through qemu_file_set_error() (well, actually it was done by hand).

Then the migration thread came, and we can do things asyncronously, so
we don't need to store the state in callbacks.  I did some changes long,
long ago about returning  error messages instead of:

if (error) {
   qemu_file_set_error(f, -error);
   return;
}

to

if (error) {
    return -error;
}

/* fix things to make errors negative, etc, etc, you get the idea */

But we need to fix some things to make this work.

Reception side is done through a coroutine, and then some of such things
still stand.  Moving to a thread would be a good idea at some point O:-)

> Also the question what is the semantic of qemu_file_set_error arises.
> It ain't documented and I would intuitively suspect that it's rather
> about the 'file' (that is transport) than the whole migration.

We test in the "interesting" parts if there is an error on the file,

from vmstate_load_state()

                } else {
                    ret = field->info->get(f, curr_elem, size, field);
                }
                if (ret >= 0) {
                    ret = qemu_file_get_error(f);
                }
                if (ret < 0) {
                    qemu_file_set_error(f, ret);
                    error_report("Failed to load %s:%s", vmsd->name,
                                 field->name);
                    trace_vmstate_load_field_error(field->name, ret);
                    return ret;
                }


You can see how many convolutions we do to maintain the error returned
and the one in QEMUFile in sync.  Removing the one in QEMUFile would be
a good idea, but requires lots of changes.

For starters, see that all qemu_get_* return void.

To add insult to injury:

int qemu_peek_byte(QEMUFile *f, int offset)
{
    int index = f->buf_index + offset;

    assert(!qemu_file_is_writable(f));
    assert(offset < IO_BUF_SIZE);

    if (index >= f->buf_size) {
        qemu_fill_buffer(f);
        index = f->buf_index + offset;
        if (index >= f->buf_size) {
            return 0;
        }
    }
    return f->buf[index];
}

I.e. it never returns errors, if there is nothing to read, it just
return 0.

/me stops

>
>
>
>>>>
>>>>   I *think* the migration code should spot that before it finishes
>>>> but it might carry on for a little while before it does.
>>>
>>> I will keep this patch as is, since this is one of the "should not happen"
>>> cases.

Later, Juan.