Re: [for-5.0 PATCH] ppc: Make PPCVirtualHypervisor an incomplete type

[Greg Kurz <groug@kaod.org>]

On Mon, 9 Dec 2019 15:02:38 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 12/9/19 2:28 PM, Greg Kurz wrote:
> > PPCVirtualHypervisor is an interface instance. It should never be
> > dereferenced. Drop the dummy type definition for extra safety, which
> > is the common practice with QOM interfaces.
> 
> This "common practice" is also referenced in commit 00ed3da9b5:
> 
>      xics: Minor fixes for XICSFabric interface
> 
>      Interface instances should never be directly dereferenced.  So, the 
> common
>      practice is to make them incomplete types to make sure no-one does 
> that.
>      XICSFrabric, however, had a dummy type which is less safe.
> 
>      We were also using OBJECT_CHECK() where we should have been using
>      INTERFACE_CHECK().
> 
> This indeed follow the changes from commit aa1b35b975d8:
> 
>      qom: make interface types abstract
> 
>      Interfaces don't have instance, let's make the interface type really
>      abstract to avoid confusion.
> 
> Now I can't find guidelines for this. If you don't know about it and use 
> 'git-grep', it is very confusing to see we use structures we never define.
> 

I agree that this deliberate usage of incomplete types isn't common.

> Can we document this use please?
> 

Probably we could amend the related section in the object.h header file.
Something like:

--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -200,8 +200,11 @@ typedef struct InterfaceInfo InterfaceInfo;
  *
  * Interfaces allow a limited form of multiple inheritance.  Instances are
  * similar to normal types except for the fact that are only defined by
- * their classes and never carry any state.  You can dynamically cast an object
- * to one of its #Interface types and vice versa.
+ * their classes and never carry any state.  As a consequence, a pointer to
+ * an interface instance should always be of incomplete type in order to be
+ * sure it cannot be dereferenced.
+ * You can dynamically cast an object to one of its #Interface types and vice
+ * versa.
  *
  * # Methods #
  *

And even better, we could maybe come up with a way to detect that a type
was wrongly defined with coccinelle ?

> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   target/ppc/cpu.h |    4 ----
> >   1 file changed, 4 deletions(-)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index e3e82327b723..ab7d07d66047 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1220,10 +1220,6 @@ PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr);
> >   PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr);
> >   PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc);
> >   
> > -struct PPCVirtualHypervisor {
> > -    Object parent;
> > -};
> > -
> >   struct PPCVirtualHypervisorClass {
> >       InterfaceClass parent;
> >       void (*hypercall)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
> > 
> > 
>