Re: [Qemu-devel] [PATCH v3 1/2] pvpanic: initialization cleanup

[Laszlo Ersek <lersek@redhat.com>]

On 06/19/13 17:02, Michael S. Tsirkin wrote:
> Avoid use of static variables: PC systems
> initialize pvpanic device through pvpanic_init,
> so we can simply create the fw_cfg file at that point.
> This also makes it possible to skip device
> creation completely if fw_cfg is not there, e.g. for xen -
> so the ports it reserves are not discoverable by guests.
> 
> Also, make pvpanic_init void since callers ignore return
> status anyway.
> 
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Chanes from v2:
>     skip device creation completely if !fw_cfg
>     make pvpanic_init void
> Changes from v1:
>     don't assert if !fw_cfg
> 
> 
>  hw/misc/pvpanic.c    | 30 ++++++++++++++++--------------
>  include/hw/i386/pc.h |  2 +-
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 060099b..83ed226 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -97,26 +97,28 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp)
>  {
>      ISADevice *d = ISA_DEVICE(dev);
>      PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> -    static bool port_configured;
> -    FWCfgState *fw_cfg;
>  
>      isa_register_ioport(d, &s->io, s->ioport);
> +}
>  
> -    if (!port_configured) {
> -        fw_cfg = fw_cfg_find();
> -        if (fw_cfg) {
> -            fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
> -                            g_memdup(&s->ioport, sizeof(s->ioport)),
> -                            sizeof(s->ioport));
> -            port_configured = true;
> -        }
> -    }
> +static void pvpanic_fw_cfg(ISADevice *dev, FWCfgState *fw_cfg)
> +{
> +    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> +
> +    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
> +                    g_memdup(&s->ioport, sizeof(s->ioport)),
> +                    sizeof(s->ioport));
>  }
>  
> -int pvpanic_init(ISABus *bus)
> +void pvpanic_init(ISABus *bus)
>  {
> -    isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
> -    return 0;
> +    ISADevice *dev;
> +    FWCfgState *fw_cfg = fw_cfg_find();
> +    if (!fw_cfg) {
> +        return;
> +    }
> +    dev = isa_create_simple (bus, TYPE_ISA_PVPANIC_DEVICE);
> +    pvpanic_fw_cfg(dev, fw_cfg);
>  }
>  
>  static Property pvpanic_isa_properties[] = {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ba9ba1a..458eded 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -196,7 +196,7 @@ static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>  void pc_system_firmware_init(MemoryRegion *rom_memory);
>  
>  /* pvpanic.c */
> -int pvpanic_init(ISABus *bus);
> +void pvpanic_init(ISABus *bus);
>  
>  /* e820 types */
>  #define E820_RAM        1
> 

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>