On 10.09.2017 00:07, Eduardo Habkost wrote: > On Fri, Sep 08, 2017 at 02:46:36PM +0200, David Hildenbrand wrote: >> On 08.09.2017 06:21, Thomas Huth wrote: >>> On 07.09.2017 22:13, David Hildenbrand wrote: >>>> Implemented in sclp.c, so let's move it to the right include file. >>>> Fix up one include. Do a forward declaration of CPUS390XState to fix the >>>> two sclp consoles complaining. >>>> >>>> Signed-off-by: David Hildenbrand <da...@redhat.com> >>>> --- >>>> include/hw/s390x/sclp.h | 2 ++ >>>> target/s390x/cpu.h | 1 - >>>> target/s390x/misc_helper.c | 1 + >>>> 3 files changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>>> index a72d096081..4b86a8a293 100644 >>>> --- a/include/hw/s390x/sclp.h >>>> +++ b/include/hw/s390x/sclp.h >>>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev >>>> *init_sclp_memory_hotplug_dev(void); >>>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >>>> void sclp_service_interrupt(uint32_t sccb); >>>> void raise_irq_cpu_hotplug(void); >>>> +typedef struct CPUS390XState CPUS390XState; >>>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); >>> >>> That's dangerous and likely does not work with certain versions of GCC. >>> You can't do a "forward declaration" with typedef in C, I'm afraid. See >>> for example: >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html >>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html >>> https://stackoverflow.com/questions/8367646/redefinition-of-typedef >>> >>> All this typedef'ing in QEMU is pretty bad ... we run into this problem >>> again and again. include/qemu/typedefs.h is just a work-around for this. >>> I know people like typedefs for some reasons (I used to do that, too, >>> before I realized the trouble with them), but IMHO we should rather >>> adopt the typedef-related rules from the kernel coding conventions instead: >>> >>> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs >>> >>> Thomas >>> >> >> Yes, this is really nasty. And I wasn't aware of the involved issues. >> >> This seems to be the only feasible solution (including cpu.h sounds >> wrong and will require a bunch of other includes): >> >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index a72d096081..ce80915a02 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev >> *init_sclp_memory_hotplug_dev(void); >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >> void sclp_service_interrupt(uint32_t sccb); >> void raise_irq_cpu_hotplug(void); >> +struct CPUS390XState; >> +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, >> uint32_t code); >> >> #endif > > Why not use typedefs.h? > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > --- > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index 4b86a8a293..3512bf8283 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -242,7 +242,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > void sclp_service_interrupt(uint32_t sccb); > void raise_irq_cpu_hotplug(void); > -typedef struct CPUS390XState CPUS390XState; > int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > #endif > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 39bc8351a3..9c97bffa92 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h
Using include/qemu/typedefs.h here is IMHO really ugly. Do we really want to pollute a common include file with target specific code? My preferences are first to avoid typdefs, but if we really need/want them (do we? There is no comment about this in our coding styles), I think we should rather introduce target-specific typedefs.h headers, too, for everything that is not part of the common code. Thomas