[for-5.0] ppc: Make PPCVirtualHypervisor an incomplete type
diff mbox series

Message ID 157589808041.21182.18121655959115011353.stgit@bahia.lan
State New
Headers show
Series
  • [for-5.0] ppc: Make PPCVirtualHypervisor an incomplete type
Related show

Commit Message

Greg Kurz Dec. 9, 2019, 1:28 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Dec. 9, 2019, 2:02 p.m. UTC | #1
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);
> 
>
Greg Kurz Dec. 9, 2019, 4:27 p.m. UTC | #2
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);
> > 
> > 
>
Peter Maydell Dec. 9, 2019, 4:42 p.m. UTC | #3
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
no-reply@patchew.org Dec. 9, 2019, 5:57 p.m. UTC | #4
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
no-reply@patchew.org Dec. 9, 2019, 6:08 p.m. UTC | #5
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
Greg Kurz Dec. 9, 2019, 9:04 p.m. UTC | #6
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
David Gibson Dec. 10, 2019, 12:36 a.m. UTC | #7
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);
>
Markus Armbruster Dec. 10, 2019, 10:52 a.m. UTC | #8
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.

Patch
diff mbox series

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