Re: [Qemu-devel] [PATCH 02/11] migration: Remove unneeded includes of migration/vmstate.h

[Laurent Vivier]

On 30/05/2017 17:27, Juan Quintela wrote:
> Laurent Vivier <address@hidden> wrote:
>> On 18/05/2017 19:25, Juan Quintela wrote:
>>> Signed-off-by: Juan Quintela <address@hidden>
>>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>>> ---
>>>  hw/core/qdev.c                   | 1 -
>>>  include/hw/acpi/memory_hotplug.h | 1 -
>>>  include/hw/acpi/pcihp.h          | 1 -
>>>  include/hw/pci/shpc.h            | 1 -
>>>  include/net/net.h                | 1 -
>>>  target/alpha/cpu.c               | 1 -
>>>  target/hppa/cpu.c                | 1 -
>>>  target/s390x/cpu.c               | 1 -
>>>  target/tilegx/cpu.c              | 1 -
>>>  9 files changed, 9 deletions(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 71ff95f..0ce45a2 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -37,7 +37,6 @@
>>>  #include "hw/boards.h"
>>>  #include "hw/sysbus.h"
>>>  #include "qapi-event.h"
>>> -#include "migration/vmstate.h"
>>>  
>>>  bool qdev_hotplug = false;
>>>  static bool qdev_hot_added = false;
>>
>> This one uses "VMStateDescription", "vmstate_check_only_migratable",
>> "vmstate_register_with_alias_id" and "vmstate_unregister" from
>> migration/savevm.h
> 
> #includes "hw/qdev.h"
> 
> which 1st include is
> 
> #include "hw/hw.h"
> 
> which include "migration/vmstate.h"
> 
> After this series the only include file that includes
> migration/vmstate.h is hw/hw.h.  Removing it makes no sense (IMHO)
> because it would mean including it directly in more than half of the tree.
> 
>>
>>> diff --git a/include/hw/acpi/memory_hotplug.h 
>>> b/include/hw/acpi/memory_hotplug.h
>>> index db8ebc9..77c6576 100644
>>> --- a/include/hw/acpi/memory_hotplug.h
>>> +++ b/include/hw/acpi/memory_hotplug.h
>>> @@ -3,7 +3,6 @@
>>>  
>>>  #include "hw/qdev-core.h"
>>>  #include "hw/acpi/acpi.h"
>>> -#include "migration/vmstate.h"
>>>  #include "hw/acpi/aml-build.h"
>>>  
>>>  /**
>>
>> This one uses "VMSTATE_STRUCT()" which is defined in migration/vmstate.h
> 
> #include "hw/acpi/acpi.h"
> 
> which includes:
> 
> #include "hw/acpi/acpi_dev_interface.h"
> 
> ->
> 
> #include "hw/boards.h"
> 
> ->
> 
> #include "hw/hw.h"
> 
>>> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
>>> index 04528b7..8a65f99 100644
>>> --- a/include/hw/acpi/pcihp.h
>>> +++ b/include/hw/acpi/pcihp.h
>>> @@ -28,7 +28,6 @@
>>>  #define HW_ACPI_PCIHP_H
>>>  
>>>  #include "hw/acpi/acpi.h"
>>> -#include "migration/vmstate.h"
>>>  #include "hw/hotplug.h"
>>>  
>>>  #define ACPI_PCIHP_IO_BASE_PROP "acpi-pcihp-io-base"
>>
>> This one uses "VMSTATE_STRUCT()" which is defined in migration/vmstate.h
> 
> see previous one
> 
>>> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
>>> index b208554..71e836b 100644
>>> --- a/include/hw/pci/shpc.h
>>> +++ b/include/hw/pci/shpc.h
>>> @@ -3,7 +3,6 @@
>>>  
>>>  #include "qemu-common.h"
>>>  #include "exec/memory.h"
>>> -#include "migration/vmstate.h"
>>>  #include "hw/hotplug.h"
>>>  #include "hw/pci/pci.h"
>>
>> This one uses "VMSTATE_BUFFER_UNSAFE_INFO_TEST()" which is defined in
>> migration/vmstate.h.
> 
> #include "hw/pci/pci.h"
> ->
> #include "hw/qedev.h"
> ->
> #included "hw/hw.h"
> 
>>>  
>>> diff --git a/include/net/net.h b/include/net/net.h
>>> index 99b28d5..ba663bd 100644
>>> --- a/include/net/net.h
>>> +++ b/include/net/net.h
>>> @@ -6,7 +6,6 @@
>>>  #include "qapi/qmp/qdict.h"
>>>  #include "qemu/option.h"
>>>  #include "net/queue.h"
>>> -#include "migration/vmstate.h"
>>>  #include "qapi-types.h"
>>>  
>>>  #define MAC_FMT "%02X:%02X:%02X:%02X:%02X:%02X"
>>
>> This one uses "vmstate_offset_array()" which is defined in
>> migration/vmstate.h
>>
>> Perhaps you can explain why you think they are unneeded?
> 
> In all previous ones, they arrive to hw/hw.h for means that are not
> going to be removed.  For this one, I am getting trouble finding a
> normal chain.
> 
> #include "qemu/osdep.h"
> #include "net/net.h"
> 
> with this change works.  But I am not able to find *why*.

Perhaps you can play with "cc -E"?

> 
> So, I am dropping this last bit.
> 
> Is that ok for you?

It makes sense

Thanks,
Laurent