qemu-arm
[Top][All Lists]
Advanced

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

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate


From: Peng Liang
Subject: Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate
Date: Tue, 20 Oct 2020 20:19:28 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.2.2

On 10/20/2020 7:27 PM, Peter Maydell wrote:
> On Tue, 20 Oct 2020 at 12:17, Peng Liang <liangpeng10@huawei.com> wrote:
>>
>> On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
>>> On 10/19/20 11:34 AM, Peng Liang wrote:
>>>> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
>>>> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
>>>
>>> It might be easy to add a Coccinelle script to avoid future errors.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>
>> I tried to add a Coccinelle script to add VMSTATE_END_OF_LIST() to the
>> end of VMStateDescription.fields.  For those who are not defined as
>> compound literals, it works well.  However, I cannot make it work for
>> those defined as compound literals.  And Julia doesn't think compound
>> literals are supported currently[1].  So maybe currently it's hard to
>> check the error using Coccinelle :(
> 
> I think we could probably significantly increase the chances that
> people find "missing terminator" errors in the course of normal
> debugging of their device if we made the terminator be something
> other than "is field->name NULL". That condition is quite likely
> to be satisfied by accident shortly after the real end-of-data
> (because zeroes are easy to find in memory), whereas if the condition
> is "field->flags is a magic number", for instance, then the chances of
> it being satisfied by accident are very low, and so a simple "loop
> through the field array until we find the end" is pretty likely to
> hang/crash. (If we don't already have such a loop we might need to
> add one in debug mode when a vmstate is registered.)
> 
> (This is why the REGINFO_SENTINEL used for Arm cpreg arrays is
> not a simple all-zeroes value, incidentally.)
> 
> thanks
> -- PMM
> .
> 

I found that field->flags is a bit-or field, so maybe all 0xf or other
magic number is still meaningful?  Can we use field->version_id or
field->struct_version_id as the condition?  I found they are all int
type but used as non-negative, so can we use
field->version_id/field->struct_version_id == magic number (for example,
-1) as a sentinel?

-- 
Thanks,
Peng



reply via email to

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