Re: [Qemu-devel] [PATCH v2 01/10] qdev: Replace no_user by cannot_instantiate_with_device_add_yet
Marcel Apfelbaum writes:
> On Tue, 2013-10-29 at 17:08 +0100, arm...@redhat.com wrote:
>> From: Markus Armbruster
>>
>> In an ideal world, machines can be built by wiring devices together
>> with configuration, not code. Unfortunately, that's not the world we
>> live in right now. We still have quite a few devices that need to be
>> wired up by code. If you try to device_add such a device, it'll fail
>> in sometimes mysterious ways. If you're lucky, you get an
>> unmysterious immediate crash.
>>
>> To protect users from such badness, DeviceClass member no_user used to
>> make device models unavailable with -device / device_add, but that
>> regressed in commit 18b6dad. The device model is still omitted from
>> help, but is available anyway.
>>
>> Attempts to fix the regression have been rejected with the argument
>> that the purpose of no_user isn't clear, and it's prone to misuse.
>>
>> This commit clarifies no_user's purpose. Anthony suggested to rename
>> it cannot_instantiate_with_device_add_yet_due_to_internal_bugs, which
>> I shorten somewhat to keep checkpatch happy. While there, make it
>> bool.
>>
>> Every use of cannot_instantiate_with_device_add_yet gets a FIXME
>> comment asking for rationale. The next few commits will clean them
>> all up, either by providing a rationale, or by getting rid of the use.
>>
>> With that done, the regression fix is hopefully acceptable.
>>
>> Signed-off-by: Markus Armbruster
[...]
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index e191ca0..2b571d7 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -97,7 +97,18 @@ typedef struct DeviceClass {
>> const char *fw_name;
>> const char *desc;
>> Property *props;
>> -int no_user;
>> +
>> +/*
>> + * Shall we hide this device model from -device / device_add?
>> + * All devices should support instantiation with device_add, and
>> + * this flag should not exist. But we're not there, yet. Some
>> + * devices fail to instantiate with cryptic error messages.
>> + * Others instantiate, but don't work. Exposing users to such
>> + * behavior would be cruel; this flag serves to protect them. It
>> + * should never be set without a comment explaining why it is set.
>> + * TODO remove once we're there
>> + */
>> +bool cannot_instantiate_with_device_add_yet;
>>
>> /* callbacks */
>> void (*reset)(DeviceState *dev);
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index a02c925..36f6f09 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -87,7 +87,7 @@ static void qdev_print_devinfo(DeviceClass *dc)
>> if (dc->desc) {
>> error_printf(", desc \"%s\"", dc->desc);
>> }
>> -if (dc->no_user) {
>> +if (dc->cannot_instantiate_with_device_add_yet) {
>> error_printf(", no-user");
> Maybe also the message can be changed here?
I'd rather not change output of "info qdm" without good reason.
>> }
>> error_printf("\n");
>> @@ -127,7 +127,8 @@ static void qdev_print_devinfos(bool show_no_user)
> Same question about show_no_user parameter, maybe give it a "better"
> name?
My excuse for keeping show_no_user is it controls whether "no-user" is
printed.
Regardless, I'm willing to rename if folks think it's useful.
> Seems OK to me.
>
> Reviewed-by: Marcel Apfelbaum
Thanks!
[...]
Re: [Qemu-devel] [PATCH v2 01/10] qdev: Replace no_user by cannot_instantiate_with_device_add_yet
On Tue, 2013-10-29 at 17:08 +0100, arm...@redhat.com wrote:
> From: Markus Armbruster
>
> In an ideal world, machines can be built by wiring devices together
> with configuration, not code. Unfortunately, that's not the world we
> live in right now. We still have quite a few devices that need to be
> wired up by code. If you try to device_add such a device, it'll fail
> in sometimes mysterious ways. If you're lucky, you get an
> unmysterious immediate crash.
>
> To protect users from such badness, DeviceClass member no_user used to
> make device models unavailable with -device / device_add, but that
> regressed in commit 18b6dad. The device model is still omitted from
> help, but is available anyway.
>
> Attempts to fix the regression have been rejected with the argument
> that the purpose of no_user isn't clear, and it's prone to misuse.
>
> This commit clarifies no_user's purpose. Anthony suggested to rename
> it cannot_instantiate_with_device_add_yet_due_to_internal_bugs, which
> I shorten somewhat to keep checkpatch happy. While there, make it
> bool.
>
> Every use of cannot_instantiate_with_device_add_yet gets a FIXME
> comment asking for rationale. The next few commits will clean them
> all up, either by providing a rationale, or by getting rid of the use.
>
> With that done, the regression fix is hopefully acceptable.
>
> Signed-off-by: Markus Armbruster
> ---
> hw/acpi/piix4.c| 2 +-
> hw/alpha/typhoon.c | 2 +-
> hw/arm/versatilepb.c | 2 +-
> hw/audio/pcspk.c | 2 +-
> hw/audio/pl041.c | 2 +-
> hw/block/fdc.c | 2 +-
> hw/display/pl110.c | 2 +-
> hw/dma/pl080.c | 2 +-
> hw/i2c/smbus_ich9.c| 2 +-
> hw/i386/kvm/clock.c| 2 +-
> hw/i386/kvmvapic.c | 2 +-
> hw/i386/pc.c | 2 +-
> hw/ide/piix.c | 6 +++---
> hw/ide/via.c | 2 +-
> hw/input/pckbd.c | 2 +-
> hw/input/vmmouse.c | 2 +-
> hw/intc/apic_common.c | 2 +-
> hw/intc/arm_gic.c | 2 +-
> hw/intc/arm_gic_common.c | 2 +-
> hw/intc/arm_gic_kvm.c | 2 +-
> hw/intc/i8259_common.c | 2 +-
> hw/intc/ioapic_common.c| 2 +-
> hw/intc/pl190.c| 2 +-
> hw/isa/isa-bus.c | 2 +-
> hw/isa/lpc_ich9.c | 2 +-
> hw/isa/piix4.c | 2 +-
> hw/isa/vt82c686.c | 2 +-
> hw/misc/arm_l2x0.c | 2 +-
> hw/misc/vmport.c | 2 +-
> hw/nvram/fw_cfg.c | 2 +-
> hw/pci-host/bonito.c | 4 ++--
> hw/pci-host/grackle.c | 4 ++--
> hw/pci-host/piix.c | 8
> hw/pci-host/prep.c | 4 ++--
> hw/ppc/spapr_vio.c | 2 +-
> hw/s390x/ipl.c | 2 +-
> hw/s390x/s390-virtio-bus.c | 2 +-
> hw/s390x/virtio-ccw.c | 2 +-
> hw/sd/pl181.c | 2 +-
> hw/timer/arm_mptimer.c | 2 +-
> hw/timer/hpet.c| 2 +-
> hw/timer/i8254_common.c| 2 +-
> hw/timer/m48t59.c | 2 +-
> hw/timer/mc146818rtc.c | 2 +-
> hw/timer/pl031.c | 2 +-
> include/hw/qdev-core.h | 13 -
> qdev-monitor.c | 5 +++--
> qom/cpu.c | 2 +-
> 48 files changed, 69 insertions(+), 57 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index b46bd5e..c29a703 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -508,7 +508,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void
> *data)
> k->revision = 0x03;
> k->class_id = PCI_CLASS_BRIDGE_OTHER;
> dc->desc = "PM";
> -dc->no_user = 1;
> +dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why
> */
> dc->vmsd = &vmstate_acpi;
> dc->props = piix4_pm_properties;
> }
> diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
> index 59e1bb8..60987ed 100644
> --- a/hw/alpha/typhoon.c
> +++ b/hw/alpha/typhoon.c
> @@ -938,7 +938,7 @@ static void typhoon_pcihost_class_init(ObjectClass
> *klass, void *data)
> SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> k->init = typhoon_pcihost_init;
> -dc->no_user = 1;
> +dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why
> */
> }
>
> static const TypeInfo typhoon_pcihost_info = {
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index f7e8b7e..bb0c0ba 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -390,7 +390,7 @@ static void vpb_sic_class_init(ObjectClass *klass, void
> *data)
> SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> k->init = vpb_sic_init;
> -dc->no_user = 1;
> +dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why
> */
> dc->vmsd = &vmstate_vpb_sic;
> }
>
> diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
> index 9004ce3..8e3e178 100644
> --- a/hw/audio/pcspk.c
> +++ b/hw/audio/pcspk.c
> @@ -192,7 +192,7 @@ static void pcspk_class_initfn(O
[Qemu-devel] [PATCH v2 01/10] qdev: Replace no_user by cannot_instantiate_with_device_add_yet
From: Markus Armbruster
In an ideal world, machines can be built by wiring devices together
with configuration, not code. Unfortunately, that's not the world we
live in right now. We still have quite a few devices that need to be
wired up by code. If you try to device_add such a device, it'll fail
in sometimes mysterious ways. If you're lucky, you get an
unmysterious immediate crash.
To protect users from such badness, DeviceClass member no_user used to
make device models unavailable with -device / device_add, but that
regressed in commit 18b6dad. The device model is still omitted from
help, but is available anyway.
Attempts to fix the regression have been rejected with the argument
that the purpose of no_user isn't clear, and it's prone to misuse.
This commit clarifies no_user's purpose. Anthony suggested to rename
it cannot_instantiate_with_device_add_yet_due_to_internal_bugs, which
I shorten somewhat to keep checkpatch happy. While there, make it
bool.
Every use of cannot_instantiate_with_device_add_yet gets a FIXME
comment asking for rationale. The next few commits will clean them
all up, either by providing a rationale, or by getting rid of the use.
With that done, the regression fix is hopefully acceptable.
Signed-off-by: Markus Armbruster
---
hw/acpi/piix4.c| 2 +-
hw/alpha/typhoon.c | 2 +-
hw/arm/versatilepb.c | 2 +-
hw/audio/pcspk.c | 2 +-
hw/audio/pl041.c | 2 +-
hw/block/fdc.c | 2 +-
hw/display/pl110.c | 2 +-
hw/dma/pl080.c | 2 +-
hw/i2c/smbus_ich9.c| 2 +-
hw/i386/kvm/clock.c| 2 +-
hw/i386/kvmvapic.c | 2 +-
hw/i386/pc.c | 2 +-
hw/ide/piix.c | 6 +++---
hw/ide/via.c | 2 +-
hw/input/pckbd.c | 2 +-
hw/input/vmmouse.c | 2 +-
hw/intc/apic_common.c | 2 +-
hw/intc/arm_gic.c | 2 +-
hw/intc/arm_gic_common.c | 2 +-
hw/intc/arm_gic_kvm.c | 2 +-
hw/intc/i8259_common.c | 2 +-
hw/intc/ioapic_common.c| 2 +-
hw/intc/pl190.c| 2 +-
hw/isa/isa-bus.c | 2 +-
hw/isa/lpc_ich9.c | 2 +-
hw/isa/piix4.c | 2 +-
hw/isa/vt82c686.c | 2 +-
hw/misc/arm_l2x0.c | 2 +-
hw/misc/vmport.c | 2 +-
hw/nvram/fw_cfg.c | 2 +-
hw/pci-host/bonito.c | 4 ++--
hw/pci-host/grackle.c | 4 ++--
hw/pci-host/piix.c | 8
hw/pci-host/prep.c | 4 ++--
hw/ppc/spapr_vio.c | 2 +-
hw/s390x/ipl.c | 2 +-
hw/s390x/s390-virtio-bus.c | 2 +-
hw/s390x/virtio-ccw.c | 2 +-
hw/sd/pl181.c | 2 +-
hw/timer/arm_mptimer.c | 2 +-
hw/timer/hpet.c| 2 +-
hw/timer/i8254_common.c| 2 +-
hw/timer/m48t59.c | 2 +-
hw/timer/mc146818rtc.c | 2 +-
hw/timer/pl031.c | 2 +-
include/hw/qdev-core.h | 13 -
qdev-monitor.c | 5 +++--
qom/cpu.c | 2 +-
48 files changed, 69 insertions(+), 57 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b46bd5e..c29a703 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -508,7 +508,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void
*data)
k->revision = 0x03;
k->class_id = PCI_CLASS_BRIDGE_OTHER;
dc->desc = "PM";
-dc->no_user = 1;
+dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_acpi;
dc->props = piix4_pm_properties;
}
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 59e1bb8..60987ed 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -938,7 +938,7 @@ static void typhoon_pcihost_class_init(ObjectClass *klass,
void *data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = typhoon_pcihost_init;
-dc->no_user = 1;
+dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
}
static const TypeInfo typhoon_pcihost_info = {
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index f7e8b7e..bb0c0ba 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -390,7 +390,7 @@ static void vpb_sic_class_init(ObjectClass *klass, void
*data)
SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
k->init = vpb_sic_init;
-dc->no_user = 1;
+dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->vmsd = &vmstate_vpb_sic;
}
diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index 9004ce3..8e3e178 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -192,7 +192,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void
*data)
dc->realize = pcspk_realizefn;
set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
-dc->no_user = 1;
+dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */
dc->props = pcspk_properties;
}
diff --git a/hw/audio/pl041.c b/hw/audio/pl041.c
index 539
