[Qemu-devel] [PATCH v2 0/3] typedefs: Remove scarcely used declarations

[Qemu-devel] [PATCH v2 0/3] typedefs: Remove scarcely used declarations

[Philippe Mathieu-Daudé]

Hi,

Since this file became a hot topic [*], I took previous cleanups
from last year and continue to clean it.

v2:
- reorder fw_cfg (Laszlo), remove forward declarations
- move PixelFormat to "ui/qemu-pixman.h" (Paolo)
- reword comment (Thomas)

Regards,

Phil.

[*] "HACKING: Clarify the paragraph about typedefs"
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02132.html
[*] "Fix duplicated typedefs to be able to compile with Clang in gnu99 mode"
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02117.html

Based-on: 1547460140-1473-1-git-send-email-thuth@redhat.com

Philippe Mathieu-Daudé (3):
  hw/nvram/fw_cfg: Remove various typedefs from "qemu/typedefs.h"
  ui/console: Remove MouseTransformInfo from qemu/typedefs.h
  ui/console: Remove PixelFormat from qemu/typedefs.h

 include/hw/acpi/vmgenid.h |  1 +
 include/hw/arm/virt.h     |  1 +
 include/hw/devices.h      |  1 +
 include/hw/mem/nvdimm.h   |  1 +
 include/hw/nvram/fw_cfg.h | 22 ++++++++++++----------
 include/qemu/typedefs.h   |  6 ------
 include/ui/console.h      | 14 ++------------
 include/ui/qemu-pixman.h  | 10 ++++++++++
 8 files changed, 28 insertions(+), 28 deletions(-)

-- 
2.17.2

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

[Philippe Mathieu-Daudé]

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(-)

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

[Qemu-devel] [PATCH v2 2/3] ui/console: Remove MouseTransformInfo from qemu/typedefs.h

[Philippe Mathieu-Daudé]

Header files requiring MouseTransformInfo already include "ui/console.h".

To clean "qemu/typedefs.h", move the declaration to "ui/console.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/devices.h    | 1 +
 include/qemu/typedefs.h | 1 -
 include/ui/console.h    | 4 ++--
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/devices.h b/include/hw/devices.h
index 0e27feb0c2..b5f1662225 100644
--- a/include/hw/devices.h
+++ b/include/hw/devices.h
@@ -4,6 +4,7 @@
 /* Devices that have nowhere better to go.  */
 
 #include "hw/hw.h"
+#include "ui/console.h"
 
 /* smc91c111.c */
 void smc91c111_init(NICInfo *, uint32_t, qemu_irq);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 293e47ef7c..4cda7343bf 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -47,7 +47,6 @@ typedef struct MigrationIncomingState MigrationIncomingState;
 typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
-typedef struct MouseTransformInfo MouseTransformInfo;
 typedef struct MSIMessage MSIMessage;
 typedef struct NetClientState NetClientState;
 typedef struct NetFilterState NetFilterState;
diff --git a/include/ui/console.h b/include/ui/console.h
index 0a190370ac..aa9f975544 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -65,13 +65,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
 
 void kbd_put_ledstate(int ledstate);
 
-struct MouseTransformInfo {
+typedef struct MouseTransformInfo {
     /* Touchscreen resolution */
     int x;
     int y;
     /* Calibration values as used/generated by tslib */
     int a[7];
-};
+} MouseTransformInfo;
 
 void hmp_mouse_set(Monitor *mon, const QDict *qdict);
 
-- 
2.17.2

[Qemu-devel] [PATCH v2 3/3] ui/console: Remove PixelFormat from qemu/typedefs.h

[Philippe Mathieu-Daudé]

Header files requiring PixelFormat already include "ui/qemu-pixman.h".

To clean "qemu/typedefs.h", move the declaration to "ui/qemu-pixman.h"
(removing the forward declaration).

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/typedefs.h  |  1 -
 include/ui/console.h     | 10 ----------
 include/ui/qemu-pixman.h | 10 ++++++++++
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 4cda7343bf..64b8386697 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -67,7 +67,6 @@ typedef struct PCIExpressHost PCIExpressHost;
 typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
 typedef struct PCIHostState PCIHostState;
 typedef struct PCMachineState PCMachineState;
-typedef struct PixelFormat PixelFormat;
 typedef struct PostcopyDiscardState PostcopyDiscardState;
 typedef struct Property Property;
 typedef struct PropertyInfo PropertyInfo;
diff --git a/include/ui/console.h b/include/ui/console.h
index aa9f975544..fef900db76 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -121,16 +121,6 @@ struct QemuConsoleClass {
 
 #define QEMU_ALLOCATED_FLAG     0x01
 
-struct PixelFormat {
-    uint8_t bits_per_pixel;
-    uint8_t bytes_per_pixel;
-    uint8_t depth; /* color depth in bits */
-    uint32_t rmask, gmask, bmask, amask;
-    uint8_t rshift, gshift, bshift, ashift;
-    uint8_t rmax, gmax, bmax, amax;
-    uint8_t rbits, gbits, bbits, abits;
-};
-
 typedef struct DisplaySurface {
     pixman_format_code_t format;
     pixman_image_t *image;
diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index b7c82d17fc..0668109305 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -53,6 +53,16 @@
 
 /* -------------------------------------------------------------------- */
 
+typedef struct PixelFormat {
+    uint8_t bits_per_pixel;
+    uint8_t bytes_per_pixel;
+    uint8_t depth; /* color depth in bits */
+    uint32_t rmask, gmask, bmask, amask;
+    uint8_t rshift, gshift, bshift, ashift;
+    uint8_t rmax, gmax, bmax, amax;
+    uint8_t rbits, gbits, bbits, abits;
+} PixelFormat;
+
 PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format);
 pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian);
 pixman_format_code_t qemu_drm_format_to_pixman(uint32_t drm_format);
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 2/3] ui/console: Remove MouseTransformInfo from qemu/typedefs.h

[Gerd Hoffmann]

On Mon, Jan 14, 2019 at 02:08:28PM +0100, Philippe Mathieu-Daudé wrote:
> Header files requiring MouseTransformInfo already include "ui/console.h".
> 
> To clean "qemu/typedefs.h", move the declaration to "ui/console.h"
> (removing the forward declaration).

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/devices.h    | 1 +
>  include/qemu/typedefs.h | 1 -
>  include/ui/console.h    | 4 ++--
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/devices.h b/include/hw/devices.h
> index 0e27feb0c2..b5f1662225 100644
> --- a/include/hw/devices.h
> +++ b/include/hw/devices.h
> @@ -4,6 +4,7 @@
>  /* Devices that have nowhere better to go.  */
>  
>  #include "hw/hw.h"
> +#include "ui/console.h"
>  
>  /* smc91c111.c */
>  void smc91c111_init(NICInfo *, uint32_t, qemu_irq);
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 293e47ef7c..4cda7343bf 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -47,7 +47,6 @@ typedef struct MigrationIncomingState MigrationIncomingState;
>  typedef struct MigrationState MigrationState;
>  typedef struct Monitor Monitor;
>  typedef struct MonitorDef MonitorDef;
> -typedef struct MouseTransformInfo MouseTransformInfo;
>  typedef struct MSIMessage MSIMessage;
>  typedef struct NetClientState NetClientState;
>  typedef struct NetFilterState NetFilterState;
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 0a190370ac..aa9f975544 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -65,13 +65,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
>  
>  void kbd_put_ledstate(int ledstate);
>  
> -struct MouseTransformInfo {
> +typedef struct MouseTransformInfo {
>      /* Touchscreen resolution */
>      int x;
>      int y;
>      /* Calibration values as used/generated by tslib */
>      int a[7];
> -};
> +} MouseTransformInfo;
>  
>  void hmp_mouse_set(Monitor *mon, const QDict *qdict);
>  
> -- 
> 2.17.2
> 

Re: [Qemu-devel] [PATCH v2 3/3] ui/console: Remove PixelFormat from qemu/typedefs.h

[Gerd Hoffmann]

On Mon, Jan 14, 2019 at 02:08:29PM +0100, Philippe Mathieu-Daudé wrote:
> Header files requiring PixelFormat already include "ui/qemu-pixman.h".
> 
> To clean "qemu/typedefs.h", move the declaration to "ui/qemu-pixman.h"
> (removing the forward declaration).

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/typedefs.h  |  1 -
>  include/ui/console.h     | 10 ----------
>  include/ui/qemu-pixman.h | 10 ++++++++++
>  3 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 4cda7343bf..64b8386697 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -67,7 +67,6 @@ typedef struct PCIExpressHost PCIExpressHost;
>  typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
>  typedef struct PCIHostState PCIHostState;
>  typedef struct PCMachineState PCMachineState;
> -typedef struct PixelFormat PixelFormat;
>  typedef struct PostcopyDiscardState PostcopyDiscardState;
>  typedef struct Property Property;
>  typedef struct PropertyInfo PropertyInfo;
> diff --git a/include/ui/console.h b/include/ui/console.h
> index aa9f975544..fef900db76 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -121,16 +121,6 @@ struct QemuConsoleClass {
>  
>  #define QEMU_ALLOCATED_FLAG     0x01
>  
> -struct PixelFormat {
> -    uint8_t bits_per_pixel;
> -    uint8_t bytes_per_pixel;
> -    uint8_t depth; /* color depth in bits */
> -    uint32_t rmask, gmask, bmask, amask;
> -    uint8_t rshift, gshift, bshift, ashift;
> -    uint8_t rmax, gmax, bmax, amax;
> -    uint8_t rbits, gbits, bbits, abits;
> -};
> -
>  typedef struct DisplaySurface {
>      pixman_format_code_t format;
>      pixman_image_t *image;
> diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
> index b7c82d17fc..0668109305 100644
> --- a/include/ui/qemu-pixman.h
> +++ b/include/ui/qemu-pixman.h
> @@ -53,6 +53,16 @@
>  
>  /* -------------------------------------------------------------------- */
>  
> +typedef struct PixelFormat {
> +    uint8_t bits_per_pixel;
> +    uint8_t bytes_per_pixel;
> +    uint8_t depth; /* color depth in bits */
> +    uint32_t rmask, gmask, bmask, amask;
> +    uint8_t rshift, gshift, bshift, ashift;
> +    uint8_t rmax, gmax, bmax, amax;
> +    uint8_t rbits, gbits, bbits, abits;
> +} PixelFormat;
> +
>  PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format);
>  pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian);
>  pixman_format_code_t qemu_drm_format_to_pixman(uint32_t drm_format);
> -- 
> 2.17.2
> 

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

[Laszlo Ersek]

Hi Phil,

On 01/14/19 14:08, 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.

No, this is not what I meant. I didn't suggest that we should remove
forward declarations.

I suggested that we should place the forward declarations *ahead of* the
first references to them:

  http://mid.mail-archive.com/32678400-47af-a842-0563-0d1a72712b4a@redhat.com

See below.

> 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(-)

[...]

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

On these lines, you have references to FWCfgState, FWCfgIoState,
FWCfgMemState. Pre-patch, that is OK, because you get the types via the
#includes, *and* the #include directives come first.

(Note: this is not about compilation safety; it is about how a human
reads this file.)

Post-patch, however:

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

the type names are introduced only later. Thus, when a human reads the
file, the type references in the OBJECT_CHECK() macros appear before the
same type names are introduced in any way.

What I meant, for v1, was simply:

> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 244ed78afafb..063375e07b2d 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -5,20 +5,20 @@
>  #include "standard-headers/linux/qemu_fw_cfg.h"
>  #include "hw/sysbus.h"
>  #include "sysemu/dma.h"
>
> +typedef struct FWCfgState FWCfgState;
> +typedef struct FWCfgIoState FWCfgIoState;
> +typedef struct FWCfgMemState FWCfgMemState;
> +
>  #define TYPE_FW_CFG     "fw_cfg"
>  #define TYPE_FW_CFG_IO  "fw_cfg_io"
>  #define TYPE_FW_CFG_MEM "fw_cfg_mem"
>
>  #define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
>  #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 FWCfgState FWCfgState;
> -typedef struct FWCfgIoState FWCfgIoState;
> -typedef struct FWCfgMemState FWCfgMemState;
> -
>  #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

Now, if you argued that this was not idiomatic for QEMU, or it was
undesirable for some other reason, that could be a valid observation,
and then we should discuss it further.

My point here is that I didn't intend my v1 suggestion as, or my R-b
for, the code that's visible in v2.

Thanks,
Laszlo

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

[Michael S. Tsirkin]

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
> 

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

[Paolo Bonzini]

On 14/01/19 16:49, Michael S. Tsirkin wrote:
> 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.

I can see why this patch can be a bit controversial.  Personally I think
it's okay either way (which generally tends to favor the status quo).

Paolo

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

[Michael S. Tsirkin]

On Mon, Jan 14, 2019 at 06:52:21PM +0100, Paolo Bonzini wrote:
> On 14/01/19 16:49, Michael S. Tsirkin wrote:
> > 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.
> 
> I can see why this patch can be a bit controversial.  Personally I think
> it's okay either way (which generally tends to favor the status quo).
> 
> Paolo

True - not something to lose sleep over.

-- 
MST