Re: [Qemu-devel] [PATCH] timer/aspeed: fix vmstate version id

["Cédric Le Goater" <clg@kaod.org>]

On 04/23/2018 11:34 AM, Peter Maydell wrote:
> On 23 April 2018 at 10:28, Cédric Le Goater <clg@kaod.org> wrote:
>> On 04/23/2018 11:12 AM, Peter Maydell wrote:
>>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>>>> index 50acbf530a3a..7df19bd9df91 100644
>>>> --- a/hw/timer/aspeed_timer.c
>>>> +++ b/hw/timer/aspeed_timer.c
>>>> @@ -498,8 +498,8 @@ static const VMStateDescription vmstate_aspeed_timer = {
>>>>
>>>>  static const VMStateDescription vmstate_aspeed_timer_state = {
>>>>      .name = "aspeed.timerctrl",
>>>> -    .version_id = 1,
>>>> -    .minimum_version_id = 1,
>>>> +    .version_id = 2,
>>>> +    .minimum_version_id = 2,
>>>>      .fields = (VMStateField[]) {
>>>>          VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
>>>>          VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
>>> Wouldn't it be simpler to just fix the incorrect value in
>>> the VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
>>> line ?
>>
>> Yes. Also.
>>
>> Or bring back all the version ids to 1, as we never supported
>> migration before.
> 
> I think it's nice to at least do the "bump version" thing, so you
> get a (hopefully comprehensible) error rather than just wrong
> data if you do try a cross version migration, 

On that topic, the error message was :

	Missing section footer for aspeed.timerctrl

which is not very comprehensible for a version mismatch issue.

Thanks,

C. 

> so I would
> vote for just fixing the one thing that was wrong: the
> number in VMSTATE_STRUCT_ARRAY is the version to be used of
> the substruct, so it didn't need to be bumped in commit
> 1d3e65aa7a; the main version numbers for vmstate_aspeed_timer
> did need to be bumped because part of the main struct changed.
> 
> thanks
> -- PMM
>