Re: [Qemu-devel] [PATCH V2 5/5] vmstate: introduce get_bufsize entry in VMStateField

[Andreas Färber]

Am 05.03.2012 09:30, schrieb Igor Mitsyanko:
> New get_bufsize 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 about 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. 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 get_bufsize callback we can calculate size of dynamic array in whichever
> way we need. We don't need .size_offset field anymore, so we can remove it 
> from
> VMState Field structure to compensate for extra memory consuption because of
> get_bufsize addition. Macros VMSTATE_VBUFFER* are modified to use new callback
> instead of .size_offset. Macro VMSTATE_BUFFER_MULTIPLY and VMFlag VMS_MULTIPLY
> are removed completely as they are now redundant.
> 
> Signed-off-by: Igor Mitsyanko <address@hidden>

Apart from this commit message being an overwhelmingly huge block of
text ;) (maybe insert an empty line or two?) this had touched on the
overall discussion of whether to pursue a declarative or imperative
approach for VMState.

So, what about adding this as a new, optional mechanism and leaving
VBUFFER / BUFFER_MULTIPLY users untouched?

Andreas

[...]
> diff --git a/vmstate.h b/vmstate.h
> index 9d3c49c..a210e08 100644
> --- a/vmstate.h
> +++ b/vmstate.h
> @@ -73,8 +73,7 @@ enum VMStateFlags {
>      VMS_BUFFER           = 0x020,  /* static sized buffer */
>      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_VBUFFER          = 0x100,  /* Buffer with dynamically calculated 
> size */
>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
>  };
> @@ -86,12 +85,12 @@ typedef struct {
>      size_t start;
>      int num;
>      size_t num_offset;
> -    size_t size_offset;
>      const VMStateInfo *info;
>      enum VMStateFlags flags;
>      const VMStateDescription *vmsd;
>      int version_id;
>      bool (*field_exists)(void *opaque, int version_id);
> +    size_t (*get_bufsize)(void *opaque, int version_id);
>  } VMStateField;
>  
>  typedef struct VMStateSubsection {
> @@ -354,34 +353,11 @@ 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(_field, _state, _version, _test, _start, 
> _get_bufsize) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
> -    .size         = (_multiply),                                      \
> -    .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_MULTIPLY,                        \
> -    .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
> -}
> -
> -#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, 
> _field_size) { \
> -    .name         = (stringify(_field)),                             \
> -    .version_id   = (_version),                                      \
> -    .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, int32_t),\
> -    .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
> -    .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
> -}
> -
> -#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, 
> _field_size) { \
> -    .name         = (stringify(_field)),                             \
> -    .version_id   = (_version),                                      \
> -    .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
> +    .get_bufsize  = (_get_bufsize),                                  \
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER,                         \
>      .offset       = offsetof(_state, _field),                        \
> @@ -570,14 +546,11 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>  #define VMSTATE_BUFFER_START_MIDDLE(_f, _s, _start) \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, NULL, _start, sizeof(typeof_field(_s, 
> _f)))
>  
> -#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
> -
> -#define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        
> \
> -    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
> +#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _get_bufsize)                 \
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _get_bufsize)
>  
> -#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
> +#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _get_bufsize)             \
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _get_bufsize)
>  
>  #define VMSTATE_BUFFER_TEST(_f, _s, _test)                            \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, _test, 0, sizeof(typeof_field(_s, _f)))

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg