hw/watchdog/wdt_diag288: Remove unnecessary includes

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

Posted by Thomas Huth 3 years, 5 months ago

Both headers, sysbus.h and module.h, are not required to compile this file.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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

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

Posted by Christian Borntraeger 3 years, 5 months ago


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 <thuth@redhat.com>

It is not a sysbus device and not a module.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  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,
>

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

Posted by Philippe Mathieu-Daudé 3 years, 5 months ago

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 <thuth@redhat.com>
> ---
>  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 <armbru@redhat.com>
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

Posted by Markus Armbruster 3 years, 5 months ago

Philippe Mathieu-Daudé <philmd@redhat.com> 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 <thuth@redhat.com>
>> ---
>>  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 <armbru@redhat.com>
> 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

Posted by Peter Maydell 3 years, 5 months ago

On Wed, 18 Nov 2020 at 14:24, Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <philmd@redhat.com> 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

Posted by Thomas Huth 3 years, 5 months ago

On 18/11/2020 15.30, Peter Maydell wrote:
> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Philippe Mathieu-Daudé <philmd@redhat.com> 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

Posted by Markus Armbruster 3 years, 5 months ago

Thomas Huth <thuth@redhat.com> writes:

> On 18/11/2020 15.30, Peter Maydell wrote:
>> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Philippe Mathieu-Daudé <philmd@redhat.com> 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

Posted by Cornelia Huck 3 years, 5 months ago

On Mon, 23 Nov 2020 11:47:25 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Thomas Huth <thuth@redhat.com> writes:
> 
> > On 18/11/2020 15.30, Peter Maydell wrote:  
> >> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster <armbru@redhat.com> wrote:  
> >>>
> >>> Philippe Mathieu-Daudé <philmd@redhat.com> 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

Posted by Thomas Huth 3 years, 5 months ago

On 23/11/2020 16.59, Cornelia Huck wrote:
> On Mon, 23 Nov 2020 11:47:25 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> Thomas Huth <thuth@redhat.com> writes:
>>
>>> On 18/11/2020 15.30, Peter Maydell wrote:  
>>>> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster <armbru@redhat.com> wrote:  
>>>>>
>>>>> Philippe Mathieu-Daudé <philmd@redhat.com> 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

Posted by Cornelia Huck 3 years, 5 months ago

On Mon, 23 Nov 2020 19:41:40 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 23/11/2020 16.59, Cornelia Huck wrote:
> > On Mon, 23 Nov 2020 11:47:25 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >   
> >> Thomas Huth <thuth@redhat.com> writes:
> >>  
> >>> On 18/11/2020 15.30, Peter Maydell wrote:    
> >>>> On Wed, 18 Nov 2020 at 14:24, Markus Armbruster <armbru@redhat.com> wrote:    
> >>>>>
> >>>>> Philippe Mathieu-Daudé <philmd@redhat.com> 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

Posted by Cornelia Huck 3 years, 5 months ago

On Wed, 18 Nov 2020 10:03:44 +0100
Thomas Huth <thuth@redhat.com> wrote:

> Both headers, sysbus.h and module.h, are not required to compile this file.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  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.