[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/3] hw/nvram/fw_cfg: Remove various typedefs
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/3] hw/nvram/fw_cfg: Remove various typedefs from "qemu/typedefs.h" |
Date: |
Mon, 14 Jan 2019 15:23:14 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
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/address@hidden
See below.
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> Reviewed-by: Laszlo Ersek <address@hidden>
> ---
> 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