[Qemu-devel] Re: optional feature

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> wrote:
> On Wed, Sep 16, 2009 at 09:21:14AM -0500, Anthony Liguori wrote:
>> Michael S. Tsirkin wrote:
>>> On Wed, Sep 16, 2009 at 09:08:59AM -0500, Anthony Liguori wrote:
>>>   
>>>> Juan Quintela wrote:
>>>>     
>>>>>>> up rtc version +1
>>>>>>> add the two fields that we need (together with rtc-td-hack value)
>>>>>>>                 
>>>>>> And why this is better? You can't migrate old VM to new qemu even if you
>>>>>> don't use rtc-td-hack on new one.
>>>>>>             
>>>>> I think the difference between us is:
>>>>> - is rtc-td-hack a hack that should only be used for some users
>>>>> - it is a valid rtc feature that should be available to everybody
>>>>> - it is independent, or it needs an rtc to have any value.
>>>>>         
>>>> We need a single table that contains the full state for the device.
>>>>
>>>> Many devices will have knobs.  There are two likely types of knobs:
>>>>
>>>> 1) something that indicates how an array of state is going to be
>>>> 2) a boolean that indicates whether a portion of state is valid
>>>>
>>>> rtc-td falls into the second category.  It makes sense to me that the 
>>>>  table state would contain a boolean to indicate whether a given set 
>>>> of  state was valid.  You may need a grouping mechanism for this.   
>>>> It  probably makes sense to do this as separate tables.  For 
>>>> instance,
>>>>
>>>> .fields = (VMStateField []) {
>>>>   VMSTATE_BOOL(td_hack, RTCState, (VMStateField[]){
>>>>        VMSTATE_INT32(irq_coalesced, RTCState),
>>>>        VMSTATE_INT32(period, RTCState),
>>>>        VMSTATE_END_OF_LIST()}),
>>>> }
>>>>
>>>> If we can't maintain backwards compatibility using this approach (we  
>>>> definitely can't for rtc-td) then we'll just have to live with that.
>>>>     
>>>
>>> We have to? Why do we?
>>
>> We could have an open loading function to load old versions of this  
>> device.  It's ugly, but there's really no other way.
>>
>>>  And not only won't we have backwards
>>> compatibility now, we will also break it and have to break it each time
>>> we add a knob.
>>>   
>>
>> No, we bump the version number and add more fields to the state.
>>
>> If we need to make crazy changes (like make a previously non-optional  
>> state, optional) then we can introduce two tables if we have to.
>>
>>> If instead we would only save/load the part of state if
>>> the knob is set, we won't have a problem.
>>>   
>>
>> The rtc device happens to support an optional feature by splitting the  
>> optional bits into a separate section.  Not every device does this  
>> though so if you want to convert other devices to this style, you'll  
>> break their backwards compatibility.
>>
>> The mechanisms are functionally the same.  One is no more expressive  
>> than the other.
>
> Yes, separate devices variant is more expressive.

> It is more modular.  With optional features A B C, versioning can not
> support saving only A and C but not B.  This is bad for example for
> backporting features between versions: if C happened to be introduced
> after B, backporting C will force backporting B.

No problem again.
You backport, and you add to the state both B and C.  You just don't
care about B values.  I leave you a name for them:

reserved0
reserved1
reserved2

And you are done.

And what is worse, what happens when you have to upgrade B and C with
new fields?  Now you have:

A, B, B', C, C'

And what options are valid?  How you differentiate between B and B', C
and C', a version number?  We are back at stage 1?

And how many features do you support?  Exponential again.

With linear version numbers you are going to have:

A
A+B
A+B+C
A+B'+C
A+B'+C'

(you can switch the 2 last ones depending the order in which changes
happen).  And that is it, no exponential cases again.  we added 4
features and we have 4 new versions.  It looks very linear to me.
And we can still load all the previous versions.

> But you can support it if you put each one in a migration container.

My opinion: We don't even want to think about this.


> if you are not saving irq state, it's better
> to skip the array that create a 0 size array.

Why?

> The former works for non-array fields, the later does not,
> and you have to invent a separate valid bit.

VMStateDescription, think of it as a contract.  Would you like that the
other part would be able to insert whole paragraphs when he wants?
Without ever telling you that it changed (i.e. same version).

I don't think so.  I am sure I would preffer that it will told me
clearly that contract changed (new version), and the changes are this,
this and this.

Later, Juan.