Re: [Qemu-devel] [PATCH v2 00/12] virtio migration: simplify vmstate helper

[Halil Pasic]

On 10/06/2016 05:30 PM, Paolo Bonzini wrote:
> 
> 
> On 06/10/2016 14:55, Halil Pasic wrote:
>>
>> Let us simplify a couple of things and get rid of some code duplication.
>>
>> NOTE: This series is exploring the suggestions of Paolo (I did my best
>> to do everything as requested). I still think that we are better of with
>> a macro that with spelling out the VMStateDescription for each device
>> separately and redundantly. The LOC balance of the previous version was
>> -41, this version is at +14 because of the expanded macros.  IMHO the
>> readability benefit of spelling out the vmsd definitions is questionabe
>> (but it is beneficial if using ctags or grep). I hope for a good
>> discussion, but I can live with this version too.
>>
>> v1 --> v2:
>> * export VMStateInfo instead of helpers
>> * change semantic of VMSTATE_VIRTIO_DEVICE
>> * drop VIRTIO_DEF_DEVICE_VMSD macro, use its expansion instead
> 
> Yes, this is what I meant...  I personally like that everything is
> spelled out in
> 
> +static const VMStateDescription vmstate_virtio_blk = {
> +    .name = "virtio-blk",
> +    .minimum_version_id = 2,
> +    .version_id = 2,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_VIRTIO_DEVICE,
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> 

Is a valid point :). There is one important thing that is not spelled
out here, and that's the state specific to virtio_blk. In my opinion
this definition suggests that virtio_blk has no state beyond its
base's (in therms of inheritance, that is virtio core+transport) state, 
and this is wrong. That is probably the main reason why I prefer
the 'magic macro' (others are my preference for the concise, and for
making similar stuff look similar, and dissimilar things look different).

This however can be remedied if we do:
in virtio.h:
extern const VMStateField virtio_vmstate_fields[];
in virtio.c:
const VMStateField virtio_vmstate_fields[] = {
    VMSTATE_VIRTIO_DEVICE,
    VMSTATE_END_OF_LIST()
};

in virtio-blk.c:
static const VMStateDescription vmstate_virtio_blk = {
    .name = "virtio-blk",
    .minimum_version_id = 2,
    .version_id = 2,
    .fields = virtio_vmstate_fields,
};

And would need to fix const correctness for VMStateField in
vmstate.c and vmstate.h.

I have never thought of this because of the const but right
how I feel like I like this option the best:
* it works with grep and ctags
* its absolutely flexible
* the oddity of irtio_vmstate_fields can be documented at
  the declaration/definition site
* we could later use VMSTATE_VIRTIO_DEVICE with the usual VMSTATE_PCI_DEVICE
  like semantic for the new devices once we establish a new migration schema
  in which the the derived specifies its state using the fields member of
  its vmsd
* redundancy is minimal:
** having separate control over minimum_version_id and version_id
   seems appropriate to me and having it written out improves readability
** establishing a naming convention for the vmsd is not so important and
   the one line we can spare there with the macro is not worth it

What do you thing guys should I make a v3 along this path?

Cheers,
Halil