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

[Greg Kurz <groug@kaod.org>]

On Mon, 9 Dec 2019 16:42:39 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 9 Dec 2019 at 16:28, Greg Kurz <groug@kaod.org> wrote:
> >
> > 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.
> 
> It might be helpful to add here the concrete details of how to do that,
> so people don't have to look up what an incomplete type is:
> 
> "That is, you should define the 'typedef struct SomethingIf SomethingIf'
> so that you can pass around 'SomethingIf *si' arguments, but not define
> a 'struct SomethingIf { ... }'. The only things you can validly do with
> a 'SomethingIf *' are to pass it as an argument to a method on its corresponding
> SomethingIfClass, or to dynamically cast the interface pointer to a pointer
> to the concrete object which is implementing the interface."
> 
> ?
> 
> > + * You can dynamically cast an object to one of its #Interface types and vice
> > + * versa.
> 
> ...though that last part is then kind of awkwardly similar to this sentence.
> There's probably better wording possible than what I suggest above.
> 

What about ?

  * 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.  That is, you should define the
+ * 'typedef struct SomethingIf SomethingIf' so that you can pass around
+ * 'SomethingIf *si' arguments, but not define a 'struct SomethingIf { ... }'.
+ * The only things you can validly do with a 'SomethingIf *' are to pass it as
+ * an argument to a method on its corresponding SomethingIfClass, or to
+ * dynamically cast it to an object that implements the interface.

> thanks
> -- PMM