Patchwork [1/3] vmstate: introduce calc_size VMStateField

login
register
mail settings
Submitter Mitsyanko Igor
Date Dec. 26, 2011, 10:03 a.m.
Message ID <1324893824-13558-2-git-send-email-i.mitsyanko@samsung.com>
Download mbox | patch
Permalink /patch/133212/
State New
Headers show

Comments

Mitsyanko Igor - Dec. 26, 2011, 10:03 a.m.
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>
---
 hw/hw.h  |   14 +++++++-------
 savevm.c |   14 ++++++++------
 2 files changed, 15 insertions(+), 13 deletions(-)
Peter Maydell - Dec. 26, 2011, 3:20 p.m.
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.

-- PMM
Mitsyanko Igor - Dec. 27, 2011, 8:11 a.m.
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.
Andreas Färber - Dec. 27, 2011, 1:10 p.m.
Am 27.12.2011 09:11, schrieb Mitsyanko Igor:
> On 12/26/2011 07:20 PM, Peter Maydell wrote:
>> On 26 December 2011 10:03, Mitsyanko Igor<i.mitsyanko@samsung.com> 
>> wrote:
>>> 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.)

It's not really a Coding Style issue though (just an aesthetic one),
what's meant by the quote is to fix braces around the lines you touch.

I'd suggest to put it in a separate preceding patch, then it gets fixed
and we can still better see what you're changing here.

(I'm in need of a mechanism like this for AHCI so please cc Juan and me
on v2 of this patch.)

Andreas
Mitsyanko Igor - Dec. 28, 2011, 7:41 a.m.
On 12/27/2011 05:10 PM, Andreas Färber wrote:
> Am 27.12.2011 09:11, schrieb Mitsyanko Igor:
>> On 12/26/2011 07:20 PM, Peter Maydell wrote:
>>> On 26 December 2011 10:03, Mitsyanko Igor<i.mitsyanko@samsung.com>
>>> wrote:
>>>> 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.)
>
> It's not really a Coding Style issue though (just an aesthetic one),
> what's meant by the quote is to fix braces around the lines you touch.
>
> I'd suggest to put it in a separate preceding patch, then it gets fixed
> and we can still better see what you're changing here.

If v2 of this patch will be accepted and we will drop VMS_MULTIPLY, I
could add required spaces together with changing values given to 
VMS_VARRAY_UINT8 and VMS_VARRAY_UINT32 flags. Or this can be done by 
Paolo Bonzini in his "split hw/hw.h" patch, I cc him just in case he 
wouldn't mind to do this.

> (I'm in need of a mechanism like this for AHCI so please cc Juan and me
> on v2 of this patch.)
>
> Andreas
>

I'm also considering to change local size variable in 
vmstate_load_state() and vmstate_save_state() to size_t, it makes no 
sense to convert size_t field->size to intermediate int type just to 
pass it again as size_t to get() function.

Patch

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 */
 };
 
 typedef struct {
@@ -321,6 +321,7 @@  typedef struct {
     const VMStateDescription *vmsd;
     int version_id;
     bool (*field_exists)(void *opaque, int version_id);
+    int (*calc_size)(void *opaque, int version_id);
 } VMStateField;
 
 typedef struct VMStateSubsection {
@@ -584,14 +585,13 @@  extern const VMStateInfo vmstate_info_unused_buffer;
     .offset       = vmstate_offset_buffer(_state, _field) + _start,  \
 }
 
-#define VMSTATE_BUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
+#define VMSTATE_VBUFFER_CALCSIZE(_field, _state, _version, _test, _start, _calc_size) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
     .field_exists = (_test),                                         \
-    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
-    .size         = (_multiply),                                      \
+    .calc_size    = (_calc_size),                                    \
     .info         = &vmstate_info_buffer,                            \
-    .flags        = VMS_VBUFFER|VMS_MULTIPLY,                        \
+    .flags        = VMS_VBUFFER|VMS_CALC_SIZE|VMS_POINTER,           \
     .offset       = offsetof(_state, _field),                        \
     .start        = (_start),                                        \
 }
diff --git a/savevm.c b/savevm.c
index f153c25..19f8985 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1412,9 +1412,10 @@  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
             int size = field->size;
 
             if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
+                if (field->flags & VMS_CALC_SIZE) {
+                    size = field->calc_size(opaque, vmsd->version_id);
+                } else {
+                    size = *(int32_t *)(opaque+field->size_offset);
                 }
             }
             if (field->flags & VMS_ARRAY) {
@@ -1476,9 +1477,10 @@  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
             int size = field->size;
 
             if (field->flags & VMS_VBUFFER) {
-                size = *(int32_t *)(opaque+field->size_offset);
-                if (field->flags & VMS_MULTIPLY) {
-                    size *= field->size;
+                if (field->flags & VMS_CALC_SIZE) {
+                    size = field->calc_size(opaque, vmsd->version_id);
+                } else {
+                    size = *(int32_t *)(opaque+field->size_offset);
                 }
             }
             if (field->flags & VMS_ARRAY) {