Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-24 Thread Cornelia Huck 
On Mon, 23 Nov 2020 19:41:40 +0100
Thomas Huth  wrote:

> On 23/11/2020 16.59, Cornelia Huck wrote:
> > On Mon, 23 Nov 2020 11:47:25 +0100
> > Markus Armbruster  wrote:
> >   
> >> Thomas Huth  writes:
> >>  
> >>> On 18/11/2020 15.30, Peter Maydell wrote:
>  On Wed, 18 Nov 2020 at 14:24, Markus Armbruster  
>  wrote:
> >
> > Philippe Mathieu-Daudé  writes:
> >
> >> On 11/18/20 10:03 AM, Thomas Huth wrote:
> >>> Both headers, sysbus.h and module.h, are not required to compile this 
> >>> file.
> >
> > module.h is: it defines type_init().
>  
> >>>  #include "qemu/timer.h"
> >>>  #include "hw/watchdog/wdt_diag288.h"
> >>>  #include "migration/vmstate.h"
> >>>  #include "qemu/log.h"
> >>> -#include "qemu/module.h"
> >>
> >> Cc'ing Markus because of:
>  
> >> Include qemu/module.h where needed, drop it from qemu-common.h
> >
> > If it still compiles and links, it must get it via some other header.   
> >  
> 
>  Yes: wdt_diag288.c -> include/hw/watchdog/wdt_diag288.h ->
>   include/qom/object.h -> include/qemu/module.h
> >>>
> >>> So what's now our expectation here? Should every file that uses 
> >>> type_init()
> >>> also include module.h ? That's IMHO not very intuitive...
> >>> Or are we fine that type_init() is provided by qom/object.h which needs to
> >>> be pulled in by every device sooner or later anyway?
> >>
> >> I think it's okay to rely on indirect inclusion.  
> > 
> > So, what's the final verdict? Maybe just tweak the description?
> > 
> > "Neither sysbus.h nor module.h are required to compile this file.
> > diag288 is not a sysbus device, and module.h (for type_init) is
> > included eventually through qom/object.h."  
> 
> Yes, I think that's the way to go. Could you update the description when
> picking up the patch, or shall I send a v2?

No need for a v2, I queued it to s390-next with an updated description.

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-24 Thread Cornelia Huck 
On Wed, 18 Nov 2020 10:03:44 +0100
Thomas Huth  wrote:

> Both headers, sysbus.h and module.h, are not required to compile this file.
> 
> Signed-off-by: Thomas Huth 
> ---
>  hw/watchdog/wdt_diag288.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
> index 71a945f0bd..4c4b6a6ab7 100644
> --- a/hw/watchdog/wdt_diag288.c
> +++ b/hw/watchdog/wdt_diag288.c
> @@ -14,12 +14,10 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/watchdog.h"
> -#include "hw/sysbus.h"
>  #include "qemu/timer.h"
>  #include "hw/watchdog/wdt_diag288.h"
>  #include "migration/vmstate.h"
>  #include "qemu/log.h"
> -#include "qemu/module.h"
>  
>  static WatchdogTimerModel model = {
>  .wdt_name = TYPE_WDT_DIAG288,

Thanks, applied.

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-23 Thread Thomas Huth 
On 23/11/2020 16.59, Cornelia Huck wrote:
> On Mon, 23 Nov 2020 11:47:25 +0100
> Markus Armbruster  wrote:
> 
>> Thomas Huth  writes:
>>
>>> On 18/11/2020 15.30, Peter Maydell wrote:  
 On Wed, 18 Nov 2020 at 14:24, Markus Armbruster  wrote: 
  
>
> Philippe Mathieu-Daudé  writes:
>  
>> On 11/18/20 10:03 AM, Thomas Huth wrote:  
>>> Both headers, sysbus.h and module.h, are not required to compile this 
>>> file.  
>
> module.h is: it defines type_init().  
   
>>>  #include "qemu/timer.h"
>>>  #include "hw/watchdog/wdt_diag288.h"
>>>  #include "migration/vmstate.h"
>>>  #include "qemu/log.h"
>>> -#include "qemu/module.h"  
>>
>> Cc'ing Markus because of:  
   
>> Include qemu/module.h where needed, drop it from qemu-common.h  
>
> If it still compiles and links, it must get it via some other header.  

 Yes: wdt_diag288.c -> include/hw/watchdog/wdt_diag288.h ->
  include/qom/object.h -> include/qemu/module.h  
>>>
>>> So what's now our expectation here? Should every file that uses type_init()
>>> also include module.h ? That's IMHO not very intuitive...
>>> Or are we fine that type_init() is provided by qom/object.h which needs to
>>> be pulled in by every device sooner or later anyway?  
>>
>> I think it's okay to rely on indirect inclusion.
> 
> So, what's the final verdict? Maybe just tweak the description?
> 
> "Neither sysbus.h nor module.h are required to compile this file.
> diag288 is not a sysbus device, and module.h (for type_init) is
> included eventually through qom/object.h."

Yes, I think that's the way to go. Could you update the description when
picking up the patch, or shall I send a v2?

 Thomas

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-23 Thread Cornelia Huck 
On Mon, 23 Nov 2020 11:47:25 +0100
Markus Armbruster  wrote:

> Thomas Huth  writes:
> 
> > On 18/11/2020 15.30, Peter Maydell wrote:  
> >> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster  wrote: 
> >>  
> >>>
> >>> Philippe Mathieu-Daudé  writes:
> >>>  
>  On 11/18/20 10:03 AM, Thomas Huth wrote:  
> > Both headers, sysbus.h and module.h, are not required to compile this 
> > file.  
> >>>
> >>> module.h is: it defines type_init().  
> >>   
> >  #include "qemu/timer.h"
> >  #include "hw/watchdog/wdt_diag288.h"
> >  #include "migration/vmstate.h"
> >  #include "qemu/log.h"
> > -#include "qemu/module.h"  
> 
>  Cc'ing Markus because of:  
> >>   
>  Include qemu/module.h where needed, drop it from qemu-common.h  
> >>>
> >>> If it still compiles and links, it must get it via some other header.  
> >> 
> >> Yes: wdt_diag288.c -> include/hw/watchdog/wdt_diag288.h ->
> >>  include/qom/object.h -> include/qemu/module.h  
> >
> > So what's now our expectation here? Should every file that uses type_init()
> > also include module.h ? That's IMHO not very intuitive...
> > Or are we fine that type_init() is provided by qom/object.h which needs to
> > be pulled in by every device sooner or later anyway?  
> 
> I think it's okay to rely on indirect inclusion.

So, what's the final verdict? Maybe just tweak the description?

"Neither sysbus.h nor module.h are required to compile this file.
diag288 is not a sysbus device, and module.h (for type_init) is
included eventually through qom/object.h."

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-23 Thread Markus Armbruster 
Thomas Huth  writes:

> On 18/11/2020 15.30, Peter Maydell wrote:
>> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster  wrote:
>>>
>>> Philippe Mathieu-Daudé  writes:
>>>
 On 11/18/20 10:03 AM, Thomas Huth wrote:
> Both headers, sysbus.h and module.h, are not required to compile this 
> file.
>>>
>>> module.h is: it defines type_init().
>> 
>  #include "qemu/timer.h"
>  #include "hw/watchdog/wdt_diag288.h"
>  #include "migration/vmstate.h"
>  #include "qemu/log.h"
> -#include "qemu/module.h"

 Cc'ing Markus because of:
>> 
 Include qemu/module.h where needed, drop it from qemu-common.h
>>>
>>> If it still compiles and links, it must get it via some other header.
>> 
>> Yes: wdt_diag288.c -> include/hw/watchdog/wdt_diag288.h ->
>>  include/qom/object.h -> include/qemu/module.h
>
> So what's now our expectation here? Should every file that uses type_init()
> also include module.h ? That's IMHO not very intuitive...
> Or are we fine that type_init() is provided by qom/object.h which needs to
> be pulled in by every device sooner or later anyway?

I think it's okay to rely on indirect inclusion.

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-23 Thread Thomas Huth 
On 18/11/2020 15.30, Peter Maydell wrote:
> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster  wrote:
>>
>> Philippe Mathieu-Daudé  writes:
>>
>>> On 11/18/20 10:03 AM, Thomas Huth wrote:
 Both headers, sysbus.h and module.h, are not required to compile this file.
>>
>> module.h is: it defines type_init().
> 
  #include "qemu/timer.h"
  #include "hw/watchdog/wdt_diag288.h"
  #include "migration/vmstate.h"
  #include "qemu/log.h"
 -#include "qemu/module.h"
>>>
>>> Cc'ing Markus because of:
> 
>>> Include qemu/module.h where needed, drop it from qemu-common.h
>>
>> If it still compiles and links, it must get it via some other header.
> 
> Yes: wdt_diag288.c -> include/hw/watchdog/wdt_diag288.h ->
>  include/qom/object.h -> include/qemu/module.h

So what's now our expectation here? Should every file that uses type_init()
also include module.h ? That's IMHO not very intuitive...
Or are we fine that type_init() is provided by qom/object.h which needs to
be pulled in by every device sooner or later anyway?

 Thomas

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-18 Thread Peter Maydell 
On Wed, 18 Nov 2020 at 14:24, Markus Armbruster  wrote:
>
> Philippe Mathieu-Daudé  writes:
>
> > On 11/18/20 10:03 AM, Thomas Huth wrote:
> >> Both headers, sysbus.h and module.h, are not required to compile this file.
>
> module.h is: it defines type_init().

> >>  #include "qemu/timer.h"
> >>  #include "hw/watchdog/wdt_diag288.h"
> >>  #include "migration/vmstate.h"
> >>  #include "qemu/log.h"
> >> -#include "qemu/module.h"
> >
> > Cc'ing Markus because of:

> > Include qemu/module.h where needed, drop it from qemu-common.h
>
> If it still compiles and links, it must get it via some other header.

Yes: wdt_diag288.c -> include/hw/watchdog/wdt_diag288.h ->
 include/qom/object.h -> include/qemu/module.h

thanks
-- PMM

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-18 Thread Markus Armbruster 
Philippe Mathieu-Daudé  writes:

> On 11/18/20 10:03 AM, Thomas Huth wrote:
>> Both headers, sysbus.h and module.h, are not required to compile this file.

module.h is: it defines type_init().

>> 
>> Signed-off-by: Thomas Huth 
>> ---
>>  hw/watchdog/wdt_diag288.c | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
>> index 71a945f0bd..4c4b6a6ab7 100644
>> --- a/hw/watchdog/wdt_diag288.c
>> +++ b/hw/watchdog/wdt_diag288.c
>> @@ -14,12 +14,10 @@
>>  #include "qemu/osdep.h"
>>  #include "sysemu/reset.h"
>>  #include "sysemu/watchdog.h"
>> -#include "hw/sysbus.h"
>
> OK
>
>>  #include "qemu/timer.h"
>>  #include "hw/watchdog/wdt_diag288.h"
>>  #include "migration/vmstate.h"
>>  #include "qemu/log.h"
>> -#include "qemu/module.h"
>
> Cc'ing Markus because of:
>
> commit 0b8fa32f551e863bb548a11394239239270dd3dc
> Author: Markus Armbruster 
> Date:   Thu May 23 16:35:07 2019 +0200
>
> Include qemu/module.h where needed, drop it from qemu-common.h

If it still compiles and links, it must get it via some other header.

>>  
>>  static WatchdogTimerModel model = {
>>  .wdt_name = TYPE_WDT_DIAG288,
>>

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-18 Thread Philippe Mathieu-Daudé 
On 11/18/20 10:03 AM, Thomas Huth wrote:
> Both headers, sysbus.h and module.h, are not required to compile this file.
> 
> Signed-off-by: Thomas Huth 
> ---
>  hw/watchdog/wdt_diag288.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
> index 71a945f0bd..4c4b6a6ab7 100644
> --- a/hw/watchdog/wdt_diag288.c
> +++ b/hw/watchdog/wdt_diag288.c
> @@ -14,12 +14,10 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/watchdog.h"
> -#include "hw/sysbus.h"

OK

>  #include "qemu/timer.h"
>  #include "hw/watchdog/wdt_diag288.h"
>  #include "migration/vmstate.h"
>  #include "qemu/log.h"
> -#include "qemu/module.h"

Cc'ing Markus because of:

commit 0b8fa32f551e863bb548a11394239239270dd3dc
Author: Markus Armbruster 
Date:   Thu May 23 16:35:07 2019 +0200

Include qemu/module.h where needed, drop it from qemu-common.h

>  
>  static WatchdogTimerModel model = {
>  .wdt_name = TYPE_WDT_DIAG288,
>

Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-18 Thread Christian Borntraeger 



On 18.11.20 10:03, Thomas Huth wrote:
> Both headers, sysbus.h and module.h, are not required to compile this file.
> 
> Signed-off-by: Thomas Huth 

It is not a sysbus device and not a module.

Reviewed-by: Christian Borntraeger 

> ---
>  hw/watchdog/wdt_diag288.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
> index 71a945f0bd..4c4b6a6ab7 100644
> --- a/hw/watchdog/wdt_diag288.c
> +++ b/hw/watchdog/wdt_diag288.c
> @@ -14,12 +14,10 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/watchdog.h"
> -#include "hw/sysbus.h"
>  #include "qemu/timer.h"
>  #include "hw/watchdog/wdt_diag288.h"
>  #include "migration/vmstate.h"
>  #include "qemu/log.h"
> -#include "qemu/module.h"
>  
>  static WatchdogTimerModel model = {
>  .wdt_name = TYPE_WDT_DIAG288,
>

[PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes

2020-11-18 Thread Thomas Huth 
Both headers, sysbus.h and module.h, are not required to compile this file.

Signed-off-by: Thomas Huth 
---
 hw/watchdog/wdt_diag288.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
index 71a945f0bd..4c4b6a6ab7 100644
--- a/hw/watchdog/wdt_diag288.c
+++ b/hw/watchdog/wdt_diag288.c
@@ -14,12 +14,10 @@
 #include "qemu/osdep.h"
 #include "sysemu/reset.h"
 #include "sysemu/watchdog.h"
-#include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "hw/watchdog/wdt_diag288.h"
 #include "migration/vmstate.h"
 #include "qemu/log.h"
-#include "qemu/module.h"
 
 static WatchdogTimerModel model = {
 .wdt_name = TYPE_WDT_DIAG288,
-- 
2.18.4