Re: [Qemu-devel] Issues around TYPE_INTERFACE

Re: [Qemu-devel] Issues around TYPE_INTERFACE

[Paolo Bonzini]

On 29/03/19 19:11, Markus Armbruster wrote:
>>>>> Yet they're used for less than half of the interface names:
>>>>>
>>>>>     TYPE_ISADMA
>>>>>     TYPE_MEMORY_DEVICE
>>>>>     TYPE_NMI

Could be renamed to TYPE_NMI_MONITOR_HANDLER.

>>>>>     TYPE_NVRAM
>>>>>     TYPE_PPC_VIRTUAL_HYPERVISOR
>>>>>     TYPE_STREAM_SLAVE
>>>>>     TYPE_USER_CREATABLE
>>>>>     TYPE_XICS_FABRIC
>>>>>     TYPE_XIVE_NOTIFIER
>>>>>
>>>>> Can we agree on one naming convention, and enforce it?
>>
>> I tend to like interface names that express an ability ("foo-er",
>> "foo-able").  Anything else should use a *_IF or *_IFACE suffix.
> 
> I like the _IFACE suffix.
> 
> Does eliding the _IFACE suffix for names ending with ER or ABLE improve
> readability?
> 
> "foo-er" interface types:
> 
>     INTERFACE_RDMA_PROVIDER
>     TYPE_FW_PATH_PROVIDER
>     TYPE_HOTPLUG_HANDLER
>     TYPE_INTERRUPT_STATS_PROVIDER
>     TYPE_XIVE_NOTIFIER
> 
> "foo-er" non-interface types:

Leaving out those where grammar threw a curve ball, or things that
aren't QOM types, only the following remain (some of which could be
debated further):

>     TYPE_EXYNOS4210_COMBINER
>     TYPE_FILTER_REWRITER
>     TYPE_GENERIC_LOADER
>     TYPE_PC_SPEAKER
>     TYPE_PR_MANAGER
>     TYPE_PR_MANAGER_HELPER
>     TYPE_QIO_DNS_RESOLVER
>     TYPE_QIO_NET_LISTENER
>     TYPE_XIVE_ROUTER
> 
> "foo-able" interface types:
> 
>     TYPE_USER_CREATABLE
> 
> "foo-able" non-interface types:
> 
>     TYPE_SPAPR_TCE_TABLE

Same here, doesn't really count.

Paolo

Re: [Qemu-devel] Issues around TYPE_INTERFACE

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/03/19 19:11, Markus Armbruster wrote:
>>>>>> Yet they're used for less than half of the interface names:
>>>>>>
>>>>>>     TYPE_ISADMA
>>>>>>     TYPE_MEMORY_DEVICE
>>>>>>     TYPE_NMI
>
> Could be renamed to TYPE_NMI_MONITOR_HANDLER.
>
>>>>>>     TYPE_NVRAM
>>>>>>     TYPE_PPC_VIRTUAL_HYPERVISOR
>>>>>>     TYPE_STREAM_SLAVE
>>>>>>     TYPE_USER_CREATABLE
>>>>>>     TYPE_XICS_FABRIC
>>>>>>     TYPE_XIVE_NOTIFIER
>>>>>>
>>>>>> Can we agree on one naming convention, and enforce it?
>>>
>>> I tend to like interface names that express an ability ("foo-er",
>>> "foo-able").  Anything else should use a *_IF or *_IFACE suffix.
>> 
>> I like the _IFACE suffix.
>> 
>> Does eliding the _IFACE suffix for names ending with ER or ABLE improve
>> readability?
>> 
>> "foo-er" interface types:
>> 
>>     INTERFACE_RDMA_PROVIDER
>>     TYPE_FW_PATH_PROVIDER
>>     TYPE_HOTPLUG_HANDLER
>>     TYPE_INTERRUPT_STATS_PROVIDER
>>     TYPE_XIVE_NOTIFIER
>> 
>> "foo-er" non-interface types:
>
> Leaving out those where grammar threw a curve ball, or things that
> aren't QOM types, only the following remain (some of which could be
> debated further):
>
>>     TYPE_EXYNOS4210_COMBINER
>>     TYPE_FILTER_REWRITER
>>     TYPE_GENERIC_LOADER
>>     TYPE_PC_SPEAKER
>>     TYPE_PR_MANAGER
>>     TYPE_PR_MANAGER_HELPER
>>     TYPE_QIO_DNS_RESOLVER
>>     TYPE_QIO_NET_LISTENER
>>     TYPE_XIVE_ROUTER

As long as grep doesn't understand grammar, further debate seems not
without merit :)

>> "foo-able" interface types:
>> 
>>     TYPE_USER_CREATABLE
>> 
>> "foo-able" non-interface types:
>> 
>>     TYPE_SPAPR_TCE_TABLE
>
> Same here, doesn't really count.
>
> Paolo

Re: [Qemu-devel] Issues around TYPE_INTERFACE

[Markus Armbruster]

I figure what I wrote was too long to read, so let me try again,
focusing on just the main issue, leaving the bonus messes for later.

Here's the main issue in a nutshell: (1) our interfaces can't have state
(but that's okay), (2) our conversions to interface types actually
convert to "an Object that has this interface" (and that's anything but
obvious).


Why (1) our interfaces can't have state:

Since interfaces are abstract, there's no such thing as an interface
instance.

Okay, but WTF does a conversion to an interface type *mean* then?
Recall, interface TYPE_MY_IFACE commonly defines struct type MyIface and
macro MY_IFACE(), which is a checked conversion from any instance of
Object to MyIface.  What *is* MyIface?


What (2) our conversions to interface types do:

MY_IFACE(obj) either fails its assertion or returns (MyIface *)obj.

When @obj points to an instance of MyFrob, then we can safely cast it to
any supertype of MyFrob.

MyIface is not a supertype of MyFrob, it's an interface.  WTF?

Turns out we only ever define MyIface in one of two ways:

    typedef struct MyIface MyIface;     // incomplete

    typedef struct MyIface {            // fake subtype of Object
        Object parent;
    } MyIface;

If we somehow know that only subtypes of SuperFrob have interface
MyIface, then we can exploit that and safely cast to SuperFrob * or any
of its supertypes.

If we don't (want to) know, then we can only cast to Object *.

The "fake subtype of Object" kind could save casts to Object *, but we
don't seem to use that.  Cargo cult?  Fear of not completing your types?

Regardless of how we cast or convert, we still know the object has
interface TYPE_MY_IFACE.


Example of how we actually use this: TYPE_MEMORY_DEVICE and unplug

pc_memory_unplug() converts DeviceState * to subtype PCDIMMDevice *,
checked.  Okay.

pc_dimm_unplug() converts PCDIMMDevice * to interface MemoryDeviceState
*.  Ooookay.  Note that MemoryDeviceState is incomplete.

memory_device_unplug() passes it on to its get_memory_region() method.
Since it's actually a PCDIMMDevice, this is
pc_dimm_md_get_memory_region().

pc_dimm_md_get_memory_region() converts the MemoryDeviceState * right
back to PCDIMMDevice *, checked.  This works even though
MemoryDeviceState is incomplete, because the checked conversion
PC_DIMM() accepts *any* pointer type.  It casts to Object * below the
hood, and if it isn't one, we get undefined behavior.

Re: [Qemu-devel] Issues around TYPE_INTERFACE

[Paolo Bonzini]

On 03/04/19 10:53, Markus Armbruster wrote:
> Why (1) our interfaces can't have state:
> 
> Since interfaces are abstract, there's no such thing as an interface
> instance.

Well, there is.  Since all class instances are Objects, and interfaces
are applied to classes, interface instances must be Object.  In fact,
INTERFACE_CHECK() expects that to be the case, since it needs to go
spelunking into the class.  However, it is an implementation detail as
far as the interface _user_ is concerned.

> When @obj points to an instance of MyFrob, then we can safely cast it to
> any supertype of MyFrob.
> 
> MyIface is not a supertype of MyFrob, it's an interface.  WTF?
> 
> Turns out we only ever define MyIface in one of two ways:
> 
>     typedef struct MyIface MyIface;     // incomplete
> 
>     typedef struct MyIface {            // fake subtype of Object
>         Object parent;
>     } MyIface;
> 
> If we somehow know that only subtypes of SuperFrob have interface
> MyIface, then we can exploit that and safely cast to SuperFrob * or any
> of its supertypes.

I'm not sure we want to know that.  That said, the incomplete type is
much nicer.  Anybody wants to add a BiteSizedTask.

Paolo