Re: [Qemu-devel] [PATCH v3 08/21] s390x: move sclp_service_call() to sclp.h

[David Hildenbrand <david@redhat.com>]

On 11.09.2017 12:23, Paolo Bonzini wrote:
> 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 <david@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?
> 
> See Markus's reply.  But, maybe it's even better to use S390CPU* and
> include target/s390x/cpu-qom.h, which by design provides as little
> definitions as needed.

I'll go with that approach. I'll drop the dependency from cpu-qom.h to
cpu_models.h (by moving typedefs to cpu-qom.h). This makes it compile at
hopefully should then be good enough for now.

(this approach implies dropping patch "[PATCH v3 05/21] target/s390x:
move typedef of S390CPU to its definition").

Thanks!

> 
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>


-- 

Thanks,

David