Re: [Qemu-devel] [PATCHv4 5/5] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

[Mark Cave-Ayland]

On 16/06/17 22:28, Laszlo Ersek wrote:

> This patch is generally good, but I'd like to suggest improvements:
> 
> (1) In the commit message, please mention that we are exposing the
> internals of FWCfgIoState and FWCfgMemState so that board code can map
> the MemoryRegion fields (such as comb_iomem) *by name*.
> 
> (2) FWCfgEntry need not be moved from the C file to the header file,
> since we never depend on the *size* of that structure. Instead, please
> add a single line to "include/qemu/typedefs.h":
> 
>    typedef struct FWCfgEntry FWCfgEntry;
> 
> and modify
> 
>     typedef struct FWCfgEntry {
>     ...
>     } FWCfgEntry;
> 
> in "fw_cfg.c" to just
> 
>     struct FWCfgEntry {
>     ...
>     };
> 
> Then "fw_cfg.h" can simply include "typedefs.h" and say
> 
>     ...
>     FWCfgEntry *entries[2];
>     ...
> 
> IOW, it's fine to leave FWCfgEntry an incomplete type in "fw_cfg.h".
> 
> (3) When you fix up patch #1 like I requested, removing the
> "FWCfgIoState.iobase" and "FWCfgIoState.dma_iobase" fields, please don't
> forget to update this patch as well, so that you not re-introduce those
> fields in the header file.
> 
> ... I'm positively satisfied with this series (I plan to "grant" my R-b
> for PATCH v5 5/5, with the above updates), but given that I'm no QOM
> expert by any means, I'd like someone with more QOM experience to review
> the patchset as well.

Thanks for the feedback! I've just revised the patchset based upon your
comments and will post the v5 to the list shortly.


ATB,

Mark.