Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity <avi@redhat.com>]

  On 08/23/2010 05:26 PM, Anthony Liguori wrote:
> On 08/23/2010 09:00 AM, Avi Kivity wrote:
>>  On 08/23/2010 04:48 PM, Anthony Liguori wrote:
>>>>> The fundamental issue is: every function (minus trivial ones) in 
>>>>> the device models code should have a state reference.  That state 
>>>>> reference should inherit from a DeviceState.  If this statement 
>>>>> isn't true, then the device has been modelled in qdev incorrectly.
>>>>>
>>>>> Using this test, quite a lot of the "converted" devices are being 
>>>>> modelled incorrectly.
>>>>
>>>> Is a "state reference" allowed to have a pointer to the state, or 
>>>> reach it in some other way (for example, static storage for 
>>>> singleton devices)?
>>>
>>>
>>> No.  If this was C++, then the statement would be: device have to be 
>>> implemented in terms of objects that inherit from Device.  Device is 
>>> our common base object.
>>
>> so,
>>
>>     struct MyDevicestate {
>>         struct DeviceState device_state;
>>         bool *some_bit;
>>     };
>>
>> bad, while
>>
>>     struct MyDevicestate {
>>         struct DeviceState device_state;
>>         bool some_bit;
>>     };
>>
>> good?
>
> And the next logical question is whether:
>
> struct MyDeviceState {
>    DeviceState qdev;
>    OtherState *s;
> };
>
> Is bad?  The answer would depend on whether OtherState implemented 
> methods or not.  If OtherState has no methods, it's fine.  If it has 
> methods, it's bad.

I don't see why.  As long as you can manipulate all of MyDevice's state 
via MyDeviceState methods, why do you care about OtherState at all?

>
>>
>>>
>>>> Isn't "save/restore works" an equivalent statement to "device state 
>>>> is reachable from the DeviceState"?
>>>
>>> I'm not sure I can connect the dots here as I'm not sure what 
>>> follows if your assertion is true.
>>
>> If save/restore works then all state is reachable from one point?  
>> Presumable DeviceState?
>>
>> I really don't see why the state has to be in the DeviceState 
>> subclass.  We're probably talking past each other here due to some 
>> confusion in terms.
>
> Probably.  Let's say that 'structs' are containers of primatives that 
> have no methods associated with them.  'objects' are structs that have 
> methods and potentially constructors/destructors.
>
> Devices can contain references to structs and objects.  If a Device 
> contains a reference to an object, the object should be stored in a 
> BusState which is a container of Devices.  Therefore, the object 
> should inherit from Device.

I disagree.  It's up to the author to decide whether to split a Device 
into 1 or 15 objects.

If one of the other objects is also a subclass of DeviceState, then 
you're probably violating that DeviceState's contract.  But that's a 
different (and trivial) matter.

(side point: in C no objects have constructors and methods.  in C++ all 
objects have constructors and methods, even PODs)

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