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

[Laszlo Ersek <lersek@redhat.com>]

On 06/16/13 22:59, 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.  Others don't use fw_cfg at all.  This also makes it possible to
> assert if fw_cfg is not there rather than skipping the device silently.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/misc/pvpanic.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 060099b..9ed9897 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -97,25 +97,22 @@ 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;
> -        }
> -    }
>  }
>  
>  int pvpanic_init(ISABus *bus)
>  {
> -    isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
> +    ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE);
> +    PVPanicState *s = ISA_PVPANIC_DEVICE(dev);
> +    FWCfgState *fw_cfg = fw_cfg_find();
> +
> +    assert(fw_cfg);

Won't the assert fire if:

  xen_enabled() &&
  machine != "pc-0.10" && machine != "pc-0.11" &&
  machine != "pc-0.12" && machine != "pc-0.13" &&
  machine != "pc-q35-1.4"

Because under the above condition "has_pvpanic" remains "true", but
fw_cfg is not initialized.

(pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to
"false", and claims to be "reused by xenfv", so the above condition may
be constant false.)

> +
> +    fw_cfg_add_file(fw_cfg, "etc/pvpanic-port",
> +                    g_memdup(&s->ioport, sizeof(s->ioport)),
> +                    sizeof(s->ioport));
> +
>      return 0;
>  }
>  
> 

Series looks good to me otherwise.

Thanks
Laszlo