qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Halil Pasic
Subject: Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states
Date: Tue, 18 Jul 2017 20:24:41 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0


On 07/18/2017 12:31 PM, Juan Quintela wrote:
> Halil Pasic <address@hidden> wrote:
>> On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
>>> * Christian Borntraeger (address@hidden) wrote:
>>>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
>>>>> * Halil Pasic (address@hidden) 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
> 

Hi Juan,

Many thanks for providing me with your perspective and a little history
lesson too. Knowing the history things do make much more sense. For me
this is quite often the case (history helps understanding) but at the
same time extracting the historical perspective form git is in my experience
quite time consuming in average case, so I don't try too often (without
having a very good reason).

I will keep the things you explained in mind and try to push the
migration subsystem into the right direction as far my time allows
(or at least avoid pushing in the wrong direction).

Regards,
Halil

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]