Re: [Qemu-block] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/07/2017 11:03 AM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (address@hidden) wrote:
>>
>>
>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>>>> I am not very clear about the logic in vmstate.c, but from its context in
>>>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>>>> for loop will keep working on the same element. So I just add a simple
>>>> check to pass that case, not sure if it's right but it can pass iotest
>>>> case 68 and 91 now.

[..]

>>
>> I have looked onto the issue. It affects s390x only if we
>> are running without KVM. Basically, S390CPU.irqstate is unused
>> if we do not use KVM, and thus no buffer is allocated.
>>
>> IMHO this is a missing field and the cleaner way to handle such
>> missing fields is exist. However this used to work, and I recommended
>> QuiFeng Hao to discuss the problem upstream.
>>
>> By the way, I think, if we want to go back to the old behavior
>> and support VMS_VBUFFER with size 0 and nullptr, a much
>> cleaner way to do the fix is to change the assert to:
>>
>> assert(first_elem  || !n_elems || !size)
>>
>> Obviously the other clean way to fix is to implement exists.
>>
>> I think the migration maintainers (Juan and Dave) should make a
>> call regarding if the described usage  of VMS_BUFFER is a legal
>> or an illegal one.
> 
> So is it this code in target/s390x/machine.c ?
> 
>         VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
>                                irqstate_saved_size),
> 

Yes!

> it should be legal for that to be zero length.
> It also makes sense that if that's zero length it should be OK for
> the pointer to be NULL.
> 
> I think it's best if you do change that assert.
> 

Makes sense. I think QingFeng will agree too and send a
new version soon.

Regards,
Halil

> Dave