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);
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);
>
>
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);
> >
> >
>
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
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
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.
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
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
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
© 2016 - 2024 Red Hat, Inc.