Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Eric Blake]

On 09/06/2017 04:10 PM, Eric Blake wrote:
>> I'm not a fan of including header files from other header files, so
>> changing the include order in the .c files sounds like the better
>> solution to me.
> 
> Eww. I like headers to be self-contained.  Other than stuff we get from
> osdep.h (which we know is included by EVERY .c file), I prefer that if a
> header uses another type, then it guarantees that an idempotent
> inclusion of a header that declares that type is also present in the
> header file, rather than forcing .c files to know which order to include
> things in.

The qemu tree-wide policy appears to be that a header should be
self-contained (order of inclusion in .c files shouldn't matter).  Also,
if header A only uses 'SomeType *', it can either get the definition of
SomeType from header B, or we can add the typedef to typedef.h at which
point both header A and B get the typedef from there (that is, typedef.h
is great for breaking what would otherwise be cyclic inclusion of
mutually-recursive types across different headers, as well as a way to
speed up compilation when a typedef is sufficient rather than a full
definition).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Eric Blake]

On 09/05/2017 06:03 AM, Thomas Huth wrote:
> On 05.09.2017 12:12, Thomas Huth wrote:
>> On 01.09.2017 20:03, Eric Blake wrote:
>>> Drop one more client of global_qtest by teaching all fw_cfg test
>>> functionality (invoked through alloc-pc) to pass in an explicit
>>> QTestState, adjusting all callers.  In particular, fw_cfg-test
>>> had to reorder things to create the test state prior to creating
>>> the fw_cfg.
>>>

>>> +++ b/tests/libqos/fw_cfg.h
>>> @@ -15,10 +15,12 @@
>>>
>>>
>>>  typedef struct QFWCFG QFWCFG;
>>> +typedef struct QTestState QTestState;
>>
>> Not sure, but I slightly remember that typedeffing a struct like this in
>> multiple places can cause compiler warnings or errors with certain
>> versions of GCC or clang? So a file that includes both, fw_cfg.h and
>> libqtest.h will then fail to compile?

Yes, older gcc fails to compile (my off-hand recollection is that it was
a bug in the older gcc, and not a standards-compliance issue to
encounter the same typedef twice, but I could be wrong), ergo the fixup
that you later noticed.

>>
>> I think it would be better to change the include order in the .c files
>> instead, so that libqtest.h is always included before fw_cfg.h.
> 
> Ah, well, I just saw that you also sent a fixup patch for this. Anyway,
> I'm not a fan of including header files from other header files, so
> changing the include order in the .c files sounds like the better
> solution to me.

Eww. I like headers to be self-contained.  Other than stuff we get from
osdep.h (which we know is included by EVERY .c file), I prefer that if a
header uses another type, then it guarantees that an idempotent
inclusion of a header that declares that type is also present in the
header file, rather than forcing .c files to know which order to include
things in.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Thomas Huth]

On 05.09.2017 12:12, Thomas Huth wrote:
> On 01.09.2017 20:03, Eric Blake wrote:
>> Drop one more client of global_qtest by teaching all fw_cfg test
>> functionality (invoked through alloc-pc) to pass in an explicit
>> QTestState, adjusting all callers.  In particular, fw_cfg-test
>> had to reorder things to create the test state prior to creating
>> the fw_cfg.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  tests/libqos/fw_cfg.h   | 10 ++
>>  tests/libqos/libqos.h   |  2 +-
>>  tests/libqos/malloc-pc.h|  4 ++--
>>  tests/libqos/malloc-spapr.h |  2 +-
>>  tests/libqos/malloc.h   |  1 +
>>  tests/boot-order-test.c |  6 +++---
>>  tests/e1000e-test.c |  2 +-
>>  tests/fw_cfg-test.c | 14 ++
>>  tests/ide-test.c|  2 +-
>>  tests/libqos/fw_cfg.c   | 14 --
>>  tests/libqos/libqos.c   |  2 +-
>>  tests/libqos/malloc-pc.c|  8 
>>  tests/libqos/malloc-spapr.c |  4 ++--
>>  tests/vhost-user-test.c |  2 +-
>>  14 files changed, 38 insertions(+), 35 deletions(-)
>>
>> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
>> index e8371b2317..396dd4ee1e 100644
>> --- a/tests/libqos/fw_cfg.h
>> +++ b/tests/libqos/fw_cfg.h
>> @@ -15,10 +15,12 @@
>>
>>
>>  typedef struct QFWCFG QFWCFG;
>> +typedef struct QTestState QTestState;
> 
> Not sure, but I slightly remember that typedeffing a struct like this in
> multiple places can cause compiler warnings or errors with certain
> versions of GCC or clang? So a file that includes both, fw_cfg.h and
> libqtest.h will then fail to compile?
> 
> I think it would be better to change the include order in the .c files
> instead, so that libqtest.h is always included before fw_cfg.h.

Ah, well, I just saw that you also sent a fixup patch for this. Anyway,
I'm not a fan of including header files from other header files, so
changing the include order in the .c files sounds like the better
solution to me.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all fw_cfg test
> functionality (invoked through alloc-pc) to pass in an explicit
> QTestState, adjusting all callers.  In particular, fw_cfg-test
> had to reorder things to create the test state prior to creating
> the fw_cfg.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqos/fw_cfg.h   | 10 ++
>  tests/libqos/libqos.h   |  2 +-
>  tests/libqos/malloc-pc.h|  4 ++--
>  tests/libqos/malloc-spapr.h |  2 +-
>  tests/libqos/malloc.h   |  1 +
>  tests/boot-order-test.c |  6 +++---
>  tests/e1000e-test.c |  2 +-
>  tests/fw_cfg-test.c | 14 ++
>  tests/ide-test.c|  2 +-
>  tests/libqos/fw_cfg.c   | 14 --
>  tests/libqos/libqos.c   |  2 +-
>  tests/libqos/malloc-pc.c|  8 
>  tests/libqos/malloc-spapr.c |  4 ++--
>  tests/vhost-user-test.c |  2 +-
>  14 files changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
> index e8371b2317..396dd4ee1e 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -15,10 +15,12 @@
> 
> 
>  typedef struct QFWCFG QFWCFG;
> +typedef struct QTestState QTestState;

Not sure, but I slightly remember that typedeffing a struct like this in
multiple places can cause compiler warnings or errors with certain
versions of GCC or clang? So a file that includes both, fw_cfg.h and
libqtest.h will then fail to compile?

I think it would be better to change the include order in the .c files
instead, so that libqtest.h is always included before fw_cfg.h.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[John Snow]

On 09/01/2017 02:03 PM, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all fw_cfg test
> functionality (invoked through alloc-pc) to pass in an explicit
> QTestState, adjusting all callers.  In particular, fw_cfg-test
> had to reorder things to create the test state prior to creating
> the fw_cfg.
> 
> Signed-off-by: Eric Blake 

Reviewed-by: John Snow