Re: [Qemu-devel] [PATCH for 2.1 00/97] VMState simplification (massive)

[Andreas Färber]

Am 07.04.2014 12:00, schrieb Juan Quintela:
> "Dr. David Alan Gilbert" <address@hidden> wrote:
>> * Juan Quintela (address@hidden) wrote:
>>> Hi
>>>
>>>   Look at the diffstat.  Almost all the additions are at
>>> test-vmstate.c. That is the reason why it is called a simplification.
>>>
>>> What this series does:
>>> - peter removal of version_minimum_id_old field when not needed (Peter)
>>> - cleanup: based on the previous one, I removed all the unneeded
>>>   the uses on the tree.  This should make your compiles
>>>   a couple of nanoseconds faster.
>>> - once there, fixed the indentation of the .fields line, to a canonical
>>>     .fields = (VMStateField[])
>>> - mst simplifications for vmstate engine
>>>
>>> And now, the big cleanup.
>>> - Patches only do one thing, to make easy the review.
>>>
>>> - Added test for all VMSTATE_FOO() definitions
>>>   (well, I am lying, VMSTATE_STRUCT* are still missing, will come soon)
>>> - We had two ways to make a field optional
>>>    VMSTATE_INT64_V(field, state, version)
>>>   and
>>>    VMSTATE_INT64_TEST(field, state, test)
>>>
>>>   We can do the version one with one test like:
>>>
>>> static inline bool vmstate_5_plus(void *opaque, int version_id)
>>> {
>>>     return version_id >= 5;
>>> }
>>>
>>>   and then change:
>>>   VMSTATE_INT64_V(field, state, 5);
>>>
>>>   into
>>>   VMSTATE_INT64_TEST(field, state, vmstate_5_plus);
>>
>> I'm not sure if I like this; while I'm OK with the idea of changing the
>> implementation of VMSTATE_INT64_V to use that function trick internally,
>> it seems like we're discouraging providing easy to parse/record versionining
>> info out of the tree.
> 
> That information is not exported Today.  And if we want to export them,
> export v = 5 or test == vmstat_5_plus is exactly the same dificulty.

That's not quite true. My code can easily compare an integer field in
VMStateDescription, but for any .test function callback I am in the
blind: Without executing the test on a specific set of data, I cannot
reason whether incoming data will be compatible with the VMSD.

> The information is still there, what has changed is the removal of one
> mechanism.
> 
> #define VMSTATE_UINT64_V(field, state, X) \
>      VMSTATE_UINT64_TEST(field, state, vmstate_##X##_plus)

If these were a fixed set of global test functions, it could work, by
special-casing function pointer comparisons.

> The same that we could do for maintaining the macro, we can do to export
> the information to whatever format we want.  I just wanted to remove one
> of the mechanism (the less powerful one).


On another matter, I still haven't heard from you on my QOM VMState
simplification series from last July:

https://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg01072.html

If we fix the NVEC issue I discovered in February and add qtests for the
remaining optional devices to assure there are no further dc->vmsd
overriding devices, would that approach be acceptable to you? Or do you
have a better suggestion?

Thanks,
Andreas

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