Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor

[Avi Kivity]

On 06/06/2012 11:27 AM, Anthony Liguori wrote:
> On 06/06/2012 03:45 PM, Avi Kivity wrote:
>> On 06/06/2012 02:51 AM, Anthony Liguori wrote:
>>>>> +during device construction and never changes.  This means we can
>>>>> add an
>>>>> +**_immutable** marker to it:
>>>>
>>>> Even if it does change (suppose we add a monitor command to retarget a
>>>> the serial device's chardev), it need not be migrated since it doesn't
>>>> describe guest state, only host state.  Maybe we should mark *chr _host
>>>> instead of _immutable.
>>>
>>> No, this is just another example of C's type system sucking and being
>>> ambiguous.
>>>
>>> Consider the following example:
>>>
>>> struct PS2Keyboard {
>>>      DeviceState parent;
>>>
>>>      PCKBDState _immutable *controller; // link
>>>      ...
>>> };
>>>
>>> This is definitely '_immutable' even though *something* has to marshal
>>> that PCKBDState.  That's because this is a reference to an externally
>>> managed object.  As long as references don't change due to guest
>>> initiated actions, they're immutable.
>>
>> In fact C allows you to express this:
>>
>>    const T *foo;   // *foo may not change, but foo may
>>    T * const foo;  // foo may not change, but *foo may
>>
>> Although every time I see this, I have to stop and think, which activity
>> I usually try to avoid.  So your example would be written
>>
>>    PCKBDState * _immutable controller; // link
>>
>> if we adopt the same system.
> 
> I think you meant to add a const in there, but yeah, I know.

Maybe also a _stupid.  How can one get a one line example wrong by
omitting the important word out of five?

> 
>>  But I wasn't referring to that at all.
>> Instead, some state is simply not relevant to the guest view, even
>> though it is state.  We don't have a lot of that since most host state
>> is behind nice interfaces, but think of a vga device that keeps track of
>> the host window size and alt-ctrl status.  Those are not guest state and
>> need not be migrated.
> 
> Both the host window size and alt-ctrl status are not device state. 
> They are host backend state and they ought to be stored in a different
> data structure. This is exactly how we do it (both things are stored in
> sdl.c or vnc.c).
> 
> If we're storing mutable host state in a device structure, that's a bug
> IMHO. That's sort of the premise of the QIDL annotations at least.

Ok.  But then the backend pointer is not 'const Backend * const', it's
'Backend * const' (if we don't allow retargeting).  So we can't say it's
_immutable (and it isn't).  It's _host.

> 
>>> In the case of a CharDriverState, the reference is always immutable.  If
>>> we supported changing char backends dynamically, it would not happen by
>>> changing the reference, but almost certainly by having the ability to
>>> reopen the char driver in place.  IOW, while the referenced object
>>> changes, the reference doesn't change.
>>
>> In either case, state changes.  The reason we don't send it over is
>> because it's not guest state, not because we reopen in place or not (and
>> migration shouldn't depend on how we choose to implement changing
>> chardev backends -- reopen or delete/new).
> 
> Well the goal is here not to support any possible way of separating
> guest state and host state.   The goal is here is to be very opinionated
> about how we separate guest and host state such that we can take a
> rigorous approach to migrating guest state.
> 
> I think we're actually pretty good today about separating host and guest
> state.  I can't think of an example within devices where we don't
> cleanly separate things outside of block devices.

And I agree that if we find such an example we should unexample it
quickly.  But you still have a pointer (or even object) in there that is
not immutable.  Why not mark it clearly instead of saying "well, the
important bits in it are immutable, we don't care about the rest"?

-- 
error compiling committee.c: too many arguments to function