Re: [Qemu-devel] Issues around TYPE_INTERFACE

[Markus Armbruster]

Daniel P. Berrangé <address@hidden> writes:

> On Tue, Mar 12, 2019 at 11:50:54AM +0100, Markus Armbruster wrote:
>> We have two separate type trees, object types rooted at TYPE_OBJECT, and
>> interface types at TYPE_INTERFACE.
>> 
>> Object types are fore defining ultimately concrete types, i.e. any
>> concrete type is a descendant of TYPE_OBJECT.  Interior nodes of the
>> TYPE_OBJECT tree are commonly abstract, with a few exceptions.  Leaves
>> need to be concrete to be of any use.
>> 
>> Interface types are for defining interfaces object types provide (by
>> listing them in a type's .interfaces).  TYPE_INTERFACE and all its
>> descendants are abstract.
>> 
>> Issue #1: INTERFACE_CLASS()
>> 
>>     #define OBJECT_CLASS(class) \
>>         ((ObjectClass *)(class))
>> 
>>     #define OBJECT_CLASS_CHECK(class_type, class, name) \
>>         ((class_type *)object_class_dynamic_cast_assert(OBJECT_CLASS(class), 
>> (name), \
>>                                                    __FILE__, __LINE__, 
>> __func__))
>> 
>>     #define INTERFACE_CLASS(klass) \
>>         OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
>> 
>> Shouldn't INTERFACE_CLASS() be named INTERFACE_CLASS_CHECK() for
>> consistency?
>
> I actually wonder why we even need "OBJECT_CLASS" as it exists here
> at all.
>
> $ git grep 'OBJECT_CLASS\b' | wc -l
> 38
> $ git grep 'OBJECT_CLASS_CHECK\b' | wc -l
> 196
>
> There's no obvious pattern in why OBJECT_CLASS is being favoured.
> The only compelling benefit of it is that it is faster since it
> avoids checking anything.
>
> What if we include sub-classes
>
>   $ git grep '_CLASS('  | grep -v GET_CLASS | wc -l
>   1946
>
> It looks like ${FOO}_CLASS is the popular syntax, but this is misleading
> as sub-classes don't follow the same convention - they all appear to make
> their ${FOO}_CLASS macro call OBJECT_CLASS_CHECK, as this is what object.h
> recommands as the pattern :-)
>
> #define MACHINE_CLASS(klass) \
>     OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
>
>
> IOW, I think we could/should remove OBJECT_CLASS and rename
> OBJECT_CLASS_CHECK to OBJECT_CLASS.
>
> If there are some callers that absolutely need the performance
> of doing a cast without safety checks, they can just do a
> direct cast with normal C syntax - no need for a macro.

+1

Wrapping a macro around a type cast is in bad taste more often than not.
C is a simple language.  Not-so-simple things can require more
acrobatics than we'd like to, even macro acrobatics.  On the flip side,
truly simple things can be kept simple and obvious.  Hiding them behind
macros throws that away.

If abstraction is more dear to you than simplicity and obviousness, then
that's totally fair, except for your poor, poor choice of a programming
language ;)

>> Rename would be trivial, as there are no uses.
>
> Renaming INTERFACE_CLASS becomes redundant if we fix OBJECT_CLASS

Yes.

>> Issue #2: INTERFACE_CHECK()
>> 
>>     #define OBJECT_CHECK(type, obj, name) \
>>         ((type *)object_dynamic_cast_assert(OBJECT(obj), (name), \
>>                                             __FILE__, __LINE__, __func__))
>> 
>>     #define INTERFACE_CHECK(interface, obj, name) \
>>         ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name), \
>>                                                  __FILE__, __LINE__, 
>> __func__))
>> 
>> The two are identical (except the former lacks parenthesis around obj,
>> which is a minor bug).
>> 
>> Obvious question: shouldn't we de-duplicate the code?
>> 
>> There's a more interesting question, however.  Since two two are
>> identical, they can be used interchangeably.  And since we can, we do;
>> for instance
>> 
>>     #define QAUTHZ(obj) \
>>          INTERFACE_CHECK(QAuthZ, (obj), \
>>                          TYPE_QAUTHZ)
>> 
>> even though TYPE_QAUTHZ is a sub-type of TYPE_OBJECT, not
>> TYPE_INTERFACE.  Shouldn't we prevent that somehow?  Or maybe embrace
>> they're the same and just get rid of INTERFACE_CHECK() entirely?
>
> If we wanted to keep INTERFACE_CHECK then I agree we should make it fail
> when not given an interface type, but i'm not really convinced it is
> worth it. Code is inevitably going to get these mixed up, as this QAUTHZ
> example shows. So doing a stronger check will just turn currently working
> code into broken code at runtime.
>
> I'd vote for removing INTERFACE_CHECK and using OBJECT_CHECK universally.

Slightly unclean, since interface types really aren't object types.
Tolerable.

>> Issue #3: Inconsistent naming (surprise, surprise)
>> 
>> The confusion between object and interface types is of course aggravated
>> by our usual lack of discipline in naming.  I recognize no less than
>> three naming conventions:
>> 
>>     INTERFACE_CONVENTIONAL_PCI_DEVICE
>>     INTERFACE_PCIE_DEVICE
>
> Definitely cull this convention - not having TYPE_ is a gratuitous
> diversion from the norm.
>
>> 
>>     TYPE_ACPI_DEVICE_IF
>>     TYPE_ARM_LINUX_BOOT_IF
>>     TYPE_TEST_IF
>>     TYPE_TPM_IF
>> 
>>     TYPE_IDAU_INTERFACE
>>     TYPE_IPMI_INTERFACE
>>     TYPE_PNV_XSCOM_INTERFACE
>
> I'm not so bothered by these, though they are pointless unless
> perhaps it is to avoid clash with a similarly named object
> type

Clash isn't possible: while the two type trees are separate, the type
name spaces are not.

>> Yet they're used for less than half of the interface names:
>> 
>>     TYPE_FW_PATH_PROVIDER
>>     TYPE_HOTPLUG_HANDLER
>>     TYPE_INTERRUPT_STATS_PROVIDER
>>     TYPE_ISADMA
>>     TYPE_MEMORY_DEVICE
>>     TYPE_NMI
>>     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?
>
>
> Regards,
> Daniel