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

[Peter Maydell]

On Mon, 9 Dec 2019 at 16:28, Greg Kurz <address@hidden> wrote:
>
> On Mon, 9 Dec 2019 15:02:38 +0100
> Philippe Mathieu-Daudé <address@hidden> 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.

thanks
-- PMM