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 <m...@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
