Re: [Qemu-devel] Re: optional feature

[Anthony Liguori <anthony@codemonkey.ws>]

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.  It's the difference of vN introduces these optional 
features vs expliciting introducing new sections.  What I don't like 
about the later is that these all need to be tied together in some sort 
of cohesive way.

>> I also think arrays should be expressed like this FWIW.  Today we have  
>> explicit typed arrays.  I would rather see:
>>
>> .fields = (VMStateField []) {
>>  VMSTATE_ARRAY(nirq, PCIBus, (VMStateField[]) {
>>      VMSTATE_INT32(irq_count[0], PCIBus),
>>      VMSTATE_END_OF_LIST()}),
>> }
>>     
>
> Same problem here.
>   

I don't see what the problem is.

Regards,

Anthony Liguori