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

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> 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 <address@hidden>
>> ---
>>  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

I prefer the kernel style for typedefs myself.  But it's strictly a
matter of style.  Excessive typedeffing makes code harder to understand,
it isn't wrong.  The part that's wrong is defining things more than
once, and that applies to everything, not just typedefs.

Sometimes you get away with defining something more than once.  It's
still wrong.

You're not supposed to define the same variable more than once, either
(although many C compilers let you get away with it, see -fno-common).
You define it in *one* place.  If you need to declare it, declare it in
*one* place, and make sure that place is in scope at the definition, so
the compiler can check the two match.

Likewise, you're not supposed to define the same struct tag more than
once, and you should declare it in just one place.

Likewise, you're not supposed to define (with typedef) the same type
more than once.  There is no such thing as a typedef declaration.

qemu/typedefs.h is not a work-around for a typedef-happy style.  Its
purpose is breaking inclusion cycles.  Secondary purpose is reducing the
need for non-cyclic includes.  If we didn't typedef, we'd still put our
struct declarations there.