Re: [Qemu-devel] [PATCH 1/3] vmstate: introduce calc_size VMStateField

[Mitsyanko Igor <i.mitsyanko@samsung.com>]

On 12/26/2011 07:20 PM, Peter Maydell wrote:
> On 26 December 2011 10:03, Mitsyanko Igor<i.mitsyanko@samsung.com>  wrote:
>> New calc_size field in VMStateField is supposed to help us easily add save/restore
>> support of dynamically allocated buffers in device's states.
>> There are some cases when information on size of dynamically allocated buffer is
>> already presented in specific device's state structure, but in such a form, that
>> can not be used with existing VMStateField interface. Currently, we either can get size from
>> another variable in device's state as it is with VMSTATE_VBUFFER_* macros, or we can
>> also multiply value kept in a variable by a constant with VMSTATE_BUFFER_MULTIPLY
>> macro. If we need to perform any other action, we're forced to add additional
>> variable with size information to device state structure with the only intention
>> to use it in VMStateDescription structure. This approach is not very pretty. Adding extra
>> flags to VMStateFlags enum for every other possible operation with size field
>> seems redundant, and still it would't cover cases when we need to perform a set of
>> operations to get size value.
>> With this new .calc_size field we can calculate size of dynamic array in whichever
>> way we need.
>>
>> Signed-off-by: Mitsyanko Igor<i.mitsyanko@samsung.com>
>
> It seems a bit curious that this patch removes the existing (although admittedly
> unused) VMSTATE_BUFFER_MULTIPLY but doesn't actually say so in the
> commit message.
>
>> ---
>>   hw/hw.h  |   14 +++++++-------
>>   savevm.c |   14 ++++++++------
>>   2 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/hw.h b/hw/hw.h
>> index efa04d1..8ce4475 100644
>> --- a/hw/hw.h
>> +++ b/hw/hw.h
>> @@ -303,9 +303,9 @@ enum VMStateFlags {
>>      VMS_ARRAY_OF_POINTER = 0x040,
>>      VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
>>      VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
>> -    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
>> -    VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>> -    VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
>> +    VMS_CALC_SIZE        = 0x200,  /* calculate size of dynamic buffer */
>> +    VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field */
>> +    VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field */
>
> These unrelated whitespace fixes are confusing -- please drop them.
QEMU wiki here http://wiki.qemu.org/Contribute/SubmitAPatch states that 
it's ok :)
 > It's OK to fix coding style issues in the immediate area (few lines) 
 > of the lines you're changing.)

Given some thoughts to it, I didn't like that this patch increases 
memory consumed by VMStateField. Maybe we should drop existing 
.size_offset field and replace it with generic get_buffsize callback, 
like we did in second version of this patch. It seems reasonable to me, 
even though existing .size_offset usage is pretty neat.

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com