* [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
@ 2020-11-18 9:03 Thomas Huth
2020-11-18 9:06 ` Christian Borntraeger
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Thomas Huth @ 2020-11-18 9:03 UTC (permalink / raw)
To: qemu-devel, Cornelia Huck; +Cc: Halil Pasic, Christian Borntraeger, qemu-s390x
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-18 9:03 [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes Thomas Huth
@ 2020-11-18 9:06 ` Christian Borntraeger
2020-11-18 11:06 ` Philippe Mathieu-Daudé
2020-11-24 11:03 ` Cornelia Huck
2 siblings, 0 replies; 11+ messages in thread
From: Christian Borntraeger @ 2020-11-18 9:06 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Cornelia Huck; +Cc: Halil Pasic, qemu-s390x
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,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-18 9:03 [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes Thomas Huth
2020-11-18 9:06 ` Christian Borntraeger
@ 2020-11-18 11:06 ` Philippe Mathieu-Daudé
2020-11-18 14:22 ` Markus Armbruster
2020-11-24 11:03 ` Cornelia Huck
2 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-18 11:06 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Cornelia Huck, Markus Armbruster
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x
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,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-18 11:06 ` Philippe Mathieu-Daudé
@ 2020-11-18 14:22 ` Markus Armbruster
2020-11-18 14:30 ` Peter Maydell
0 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2020-11-18 14:22 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Thomas Huth, Cornelia Huck, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
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,
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-18 14:22 ` Markus Armbruster
@ 2020-11-18 14:30 ` Peter Maydell
2020-11-23 8:16 ` Thomas Huth
0 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2020-11-18 14:30 UTC (permalink / raw)
To: Markus Armbruster
Cc: Thomas Huth, Cornelia Huck, QEMU Developers, Halil Pasic,
Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-18 14:30 ` Peter Maydell
@ 2020-11-23 8:16 ` Thomas Huth
2020-11-23 10:47 ` Markus Armbruster
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Huth @ 2020-11-23 8:16 UTC (permalink / raw)
To: Peter Maydell, Markus Armbruster
Cc: Cornelia Huck, QEMU Developers, Halil Pasic,
Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-23 8:16 ` Thomas Huth
@ 2020-11-23 10:47 ` Markus Armbruster
2020-11-23 15:59 ` Cornelia Huck
0 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2020-11-23 10:47 UTC (permalink / raw)
To: Thomas Huth
Cc: Peter Maydell, Cornelia Huck, QEMU Developers, Halil Pasic,
Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-23 10:47 ` Markus Armbruster
@ 2020-11-23 15:59 ` Cornelia Huck
2020-11-23 18:41 ` Thomas Huth
0 siblings, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2020-11-23 15:59 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Maydell, Thomas Huth, QEMU Developers, Halil Pasic,
Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé
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."
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-23 15:59 ` Cornelia Huck
@ 2020-11-23 18:41 ` Thomas Huth
2020-11-24 11:02 ` Cornelia Huck
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Huth @ 2020-11-23 18:41 UTC (permalink / raw)
To: Cornelia Huck, Markus Armbruster
Cc: Peter Maydell, QEMU Developers, Halil Pasic,
Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-23 18:41 ` Thomas Huth
@ 2020-11-24 11:02 ` Cornelia Huck
0 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2020-11-24 11:02 UTC (permalink / raw)
To: Thomas Huth
Cc: Peter Maydell, Markus Armbruster, QEMU Developers, Halil Pasic,
Christian Borntraeger, qemu-s390x, Philippe Mathieu-Daudé
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes
2020-11-18 9:03 [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes Thomas Huth
2020-11-18 9:06 ` Christian Borntraeger
2020-11-18 11:06 ` Philippe Mathieu-Daudé
@ 2020-11-24 11:03 ` Cornelia Huck
2 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2020-11-24 11:03 UTC (permalink / raw)
To: Thomas Huth; +Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-11-24 11:06 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 9:03 [PATCH] hw/watchdog/wdt_diag288: Remove unnecessary includes Thomas Huth
2020-11-18 9:06 ` Christian Borntraeger
2020-11-18 11:06 ` Philippe Mathieu-Daudé
2020-11-18 14:22 ` Markus Armbruster
2020-11-18 14:30 ` Peter Maydell
2020-11-23 8:16 ` Thomas Huth
2020-11-23 10:47 ` Markus Armbruster
2020-11-23 15:59 ` Cornelia Huck
2020-11-23 18:41 ` Thomas Huth
2020-11-24 11:02 ` Cornelia Huck
2020-11-24 11:03 ` Cornelia Huck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.