Re: [Qemu-devel] [PATCH 13/25] vmstate: port ppc cpu

[Juan Quintela <quintela@redhat.com>]

Alexander Graf <agraf@suse.de> wrote:
> On 25.10.2011, at 16:00, Juan Quintela wrote:
>
>> Added sdr1_vmstate because storing the value requires calling ppc_store_sdr1().
>> The position when the function is called also changes (I think it is save).
>
> Thanks for converting this. I'm fairly sure that vmsave is broken atm anyways and that someone will have to go through all the fields and make sure they're still working properly. However you're definitely not making the situation worse, so here's only a small comment:
>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> CC: Alexander Graf <agraf@suse.de>
>> ---
>> target-ppc/cpu.h     |    4 +-
>> target-ppc/machine.c |  245 ++++++++++++++++++--------------------------------
>> 2 files changed, 89 insertions(+), 160 deletions(-)
>> 
>> +        VMSTATE_UINTTL_ARRAY(spr, CPUState, 1024),
>
> Shouldn't sizeof(((CPUState*)NULL)->spr / sizeof(target_ulong)) work as size field?

vmstate is inconsistent about this.

It "calculates" sizes for "strings" (ar uint8_t * if your preffer), but
not for arrays.

#define VMSTATE_BUFFER_V(_f, _s, _v)                                  \
    VMSTATE_STATIC_BUFFER(_f, _s, _v, NULL, 0, sizeof(typeof_field(_s, _f)))

#define VMSTATE_BUFFER(_f, _s)                                        \
    VMSTATE_BUFFER_V(_f, _s, 0)

And I don't know what the "right" solution is:
- Putting explicit size helps detect changes on that size (good)
- But, on lots of places, we are putting there a constant:
        VMSTATE_INT32_ARRAY(tx_fifo, smc91c111_state, NUM_PACKETS),
  So, we are not going to detect changes either.

Next on my plate is to get someway of unittesting and being able to
detect this kind of changes (then we can actually drop the sizes
always).

Later, Juan.