Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)

[Thomas Huth]

On 30/07/15 17:00, Jason J. Herne wrote:
> On 07/21/2015 06:37 AM, David Hildenbrand wrote:
>>>
>>> So if I've got this code right, you send here a "header" that announces
>>> a packet with all pages ...
>>>
>>>> +    while (handled_count < total_count) {
>>>> +        cur_count = MIN(total_count - handled_count,
>>>> S390_SKEYS_BUFFER_SIZE);
>>>> +
>>>> +        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
>>>> +        if (ret < 0) {
>>>> +            error_report("S390_GET_KEYS error %d\n", ret);
>>>> +            break;
>>>
>>> ... but when an error occurs here, you suddenly stop in the middle of
>>> that "packet" with all pages ...
>>
>> Indeed, although that should never fail, we never know.
>> We don't want to overengineer the protocol but still abort migration
>> at least
>> on the loading side in that (theoretical) case.
>>
> 
> I don't have a strong opinion on this either way. I think it is fine
> just the way
> it is (for the reasons David described above). However, if people are
> worried I
> can see about writing some code that sends fake keys to the destination as
> described below. Thoughts?

If David is right and the skeyclass->get_skeys() really never fails (I
did not check), then simply do an "assert (ret == 0)" afterwards - that
way you can be sure that it really never fails. And if it ever fails,
you notice it immediately - and that's certainly way much better than
debugging the currently-wrong error handling code.

 Thomas