Re: [Qemu-devel] Issues around TYPE_INTERFACE

[Daniel P . Berrangé]

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.


> Rename would be trivial, as there are no uses.

Renaming INTERFACE_CLASS becomes redundant if we fix OBJECT_CLASS

> 
> 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.

> 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

> 
> 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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|