Re: [Qemu-devel] [PATCH v2 1/3] hw/nvram/fw_cfg: Remove various typedefs from "qemu/typedefs.h"

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jan 14, 2019 at 02:08:27PM +0100, Philippe Mathieu-Daudé wrote:
> There are only three include files requiring these typedefs, let them
> include "hw/nvram/fw_cfg.h" directly to simplify "qemu/typedefs.h".
> 
> To clean "qemu/typedefs.h", move the declarations to "hw/nvram/fw_cfg.h".
> Reorder two function declarations to avoid forward typedef declarations.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  include/hw/acpi/vmgenid.h |  1 +
>  include/hw/arm/virt.h     |  1 +
>  include/hw/mem/nvdimm.h   |  1 +
>  include/hw/nvram/fw_cfg.h | 22 ++++++++++++----------
>  include/qemu/typedefs.h   |  4 ----
>  5 files changed, 15 insertions(+), 14 deletions(-)

I am sorry I don't understand what this patchset is about.
Supposed to be a cleanup but instead it pulls
fw cfg into any user of unrelated headers.

More lines of code slower builds what's to like?

Shared typedefs is what typedefs.h is about.
These are clearly shared so let's keep it simple.


> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
> index 38586ecbdf..be53de38f1 100644
> --- a/include/hw/acpi/vmgenid.h
> +++ b/include/hw/acpi/vmgenid.h
> @@ -3,6 +3,7 @@
>  
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/qdev.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "qemu/uuid.h"
>  
>  #define VMGENID_DEVICE           "vmgenid"
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 4cc57a7ef6..fef632dcf5 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -35,6 +35,7 @@
>  #include "qemu/notify.h"
>  #include "hw/boards.h"
>  #include "hw/arm/arm.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "sysemu/kvm.h"
>  #include "hw/intc/arm_gicv3_common.h"
>  
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c5c9b3c7f8..51d3a52c4b 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -24,6 +24,7 @@
>  #define QEMU_NVDIMM_H
>  
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  
>  #define NVDIMM_DEBUG 0
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a74..cc744d5268 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -14,15 +14,12 @@
>  #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
>  #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>  
> -typedef struct fw_cfg_file FWCfgFile;
> -
>  #define FW_CFG_ORDER_OVERRIDE_VGA    70
>  #define FW_CFG_ORDER_OVERRIDE_NIC    80
>  #define FW_CFG_ORDER_OVERRIDE_USER   100
>  #define FW_CFG_ORDER_OVERRIDE_DEVICE 110
>  
> -void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order);
> -void fw_cfg_reset_order_override(FWCfgState *fw_cfg);
> +typedef struct fw_cfg_file FWCfgFile;
>  
>  typedef struct FWCfgFiles {
>      uint32_t  count;
> @@ -34,7 +31,9 @@ typedef struct fw_cfg_dma_access FWCfgDmaAccess;
>  typedef void (*FWCfgCallback)(void *opaque);
>  typedef void (*FWCfgWriteCallback)(void *opaque, off_t start, size_t len);
>  
> -struct FWCfgState {
> +typedef struct FWCfgEntry FWCfgEntry;
> +
> +typedef struct FWCfgState {
>      /*< private >*/
>      SysBusDevice parent_obj;
>      /*< public >*/
> @@ -53,17 +52,17 @@ struct FWCfgState {
>      dma_addr_t dma_addr;
>      AddressSpace *dma_as;
>      MemoryRegion dma_iomem;
> -};
> +} FWCfgState;
>  
> -struct FWCfgIoState {
> +typedef struct FWCfgIoState {
>      /*< private >*/
>      FWCfgState parent_obj;
>      /*< public >*/
>  
>      MemoryRegion comb_iomem;
> -};
> +} FWCfgIoState;
>  
> -struct FWCfgMemState {
> +typedef struct FWCfgMemState {
>      /*< private >*/
>      FWCfgState parent_obj;
>      /*< public >*/
> @@ -71,7 +70,10 @@ struct FWCfgMemState {
>      MemoryRegion ctl_iomem, data_iomem;
>      uint32_t data_width;
>      MemoryRegionOps wide_data_ops;
> -};
> +} FWCfgMemState;
> +
> +void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order);
> +void fw_cfg_reset_order_override(FWCfgState *fw_cfg);
>  
>  /**
>   * fw_cfg_add_bytes:
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 30ad8b2d8d..293e47ef7c 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -28,10 +28,6 @@ typedef struct DriveInfo DriveInfo;
>  typedef struct Error Error;
>  typedef struct EventNotifier EventNotifier;
>  typedef struct FlatView FlatView;
> -typedef struct FWCfgEntry FWCfgEntry;
> -typedef struct FWCfgIoState FWCfgIoState;
> -typedef struct FWCfgMemState FWCfgMemState;
> -typedef struct FWCfgState FWCfgState;
>  typedef struct HVFX86EmulatorState HVFX86EmulatorState;
>  typedef struct I2CBus I2CBus;
>  typedef struct I2SCodec I2SCodec;
> -- 
> 2.17.2
>