Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 08/08/2018 10:11 PM, Dr. David Alan Gilbert wrote:
> * Xiao Guangrong (guangrong.xiao@gmail.com) wrote:
>>
>>
>> On 08/08/2018 01:08 PM, Peter Xu wrote:
>>> On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
>>>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>>
>>>> ram_find_and_save_block() can return negative if any error hanppens,
>>>> however, it is completely ignored in current code
>>>
>>> Could you hint me where we'll return an error?
>>>
>>
>> I think control_save_page() may return a error condition but i am not
>> good at it ... Other places look safe _currently_. These functions were
>> designed to have error returned anyway.
> 
> ram_control_save_page's return is checked by control_save_page which
> returns true/false but sets *pages to a return value.
> 
> What I'd need to follow closely is the case where ram_control_save_page
> returns RAM_SAVE_CONTROL_DELAYED, in that case control_save_page I think
> returns with *pages=-1 and returns true.
> And I think in that case ram_save_target_page can leak that -1 - hmm.
> 
> Now, ram_save_host_page already checks for <0 and will return that,
> but I think that would potentially loop in ram_find_and_save_block; I'm
> not sure we want to change that or not!

ram_find_and_save_block() will continue the look only if ram_save_host_page
returns zero:

......
         if (found) {
             pages = ram_save_host_page(rs, &pss, last_stage);
         }
     } while (!pages && again);

IMHO, how to change the code really depends on the semantic of these functions,
based on the comments of them, returning error conditions is the current
semantic.

Another choice would be the one squashes error conditions to QEMUFile and
adapt comments and code of these functions to reflect the new semantic
clearly.

Which one do you guys prefer to? :)