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

[Thomas Huth]

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