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

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

[Greg Kurz]

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.

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);

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

[Philippe Mathieu-Daudé]

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.

Can we document this use please?

> 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);
> 
> 

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

[Greg Kurz]

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);
> > 
> > 
> 

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Greg Kurz]

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

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

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

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

Incomplete type is the closest you get to abstract class in C.

Prior discussion:
Subject: Re: Issues around TYPE_INTERFACE
Message-ID: <fdaa779c-ed79-647b-6038-3df2353fe502@redhat.com>
https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg00749.html

> Can we document this use please?

Fair.

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/157589808041.21182.18121655959115011353.stgit@bahia.lan/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5280, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-o_l3ut3r/src/docker-src.2019-12-09-12.55.49.11251] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-o_l3ut3r/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m5.957s
user    0m2.673s


The full log is available at
http://patchew.org/logs/157589808041.21182.18121655959115011353.stgit@bahia.lan/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/157589808041.21182.18121655959115011353.stgit@bahia.lan/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5280, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-o6f8wr_f/src/docker-src.2019-12-09-13.03.52.12655] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-o6f8wr_f/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    4m16.708s
user    0m2.530s


The full log is available at
http://patchew.org/logs/157589808041.21182.18121655959115011353.stgit@bahia.lan/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On Mon, Dec 09, 2019 at 02:28:00PM +0100, 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.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-5.0.

> ---
>  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);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]