Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Andreas Färber]

Am 16.09.2013 14:33, schrieb Michael S. Tsirkin:
> On Mon, Sep 16, 2013 at 08:32:13AM +0200, Andreas Färber wrote:
>> Am 15.09.2013 19:23, schrieb Michael S. Tsirkin:
>>> Add a helper macro for adding read-only properties, that works in the
>>> common case where the value is a constant.
>>>
>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>> ---
>>>
>>> I'm using this patch in my acpi work - any objections
>>> to applying it on my tree?
>>
>> Actually yes: Apart from the clang issues raised and the disturbing
>> upper-casing of arguments, this is hardcoding "int" type and NULL errp,
>> so I don't think it deserves to live in object.h as is. I do agree that
>> we could use more helper functions to deal with dynamic properties.
>>
>> So what about taking bool/string property helpers as example and putting
>> intX_t getters into object.c, using a passed-through opaque argument to
>> obtain the value? We could then have real object_property_add_int32()
>> etc. functions using the appropriate type name, with field/value pointer
>> and Error** arguments. A pointer can be assumed to hold up to uint32_t
>> values or, to keep the API more general, use a local static const
>> variable for non-field values.
> 
> This reminds me.
> address@hidden qemu]$ git grep object_property_set_bool
> backends/rng.c:    object_property_set_bool(OBJECT(s), true, "opened", errp);
> backends/tpm.c:    object_property_set_bool(OBJECT(s), true, "opened", errp);

These look like two distinct properties used once each.

[...]
> hw/core/qdev.c:    object_property_set_bool(OBJECT(dev), true, "realized", 
> &local_err);
[...]
> hw/core/qdev.c:        object_property_set_bool(obj, false, "realized", NULL);
> hw/i386/pc.c:    object_property_set_bool(OBJECT(cpu), true, "realized", 
> &local_err);
> hw/pci-host/prep.c:    object_property_set_bool(OBJECT(&s->pci_dev), true, 
> "realized", errp);
> hw/pci-host/versatile.c:    object_property_set_bool(OBJECT(&s->pci_dev), 
> true, "realized", errp)
> hw/scsi/scsi-bus.c:    object_property_set_bool(OBJECT(dev), true, 
> "realized", &err);
[...]
> target-alpha/cpu.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-arm/helper.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-cris/cpu.c:    object_property_set_bool(OBJECT(cpu), true, "realized", 
> NULL);
> target-i386/cpu.c:        object_property_set_bool(OBJECT(cpu), true, "pmu", 
> &err);
> target-i386/cpu.c:    object_property_set_bool(OBJECT(cpu), true, "realized", 
> &error);
> target-lm32/helper.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-m68k/helper.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-microblaze/translate.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-mips/translate.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-moxie/cpu.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-openrisc/cpu.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-ppc/translate_init.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", &err);
> target-s390x/helper.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-sh4/cpu.c:    object_property_set_bool(OBJECT(cpu), true, "realized", 
> NULL);
> target-sparc/cpu.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-unicore32/helper.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);
> target-xtensa/helper.c:    object_property_set_bool(OBJECT(cpu), true, 
> "realized", NULL);

Leaving the bulk for "realized".

> 
> Shouldn't we have a constant for the "realized" string?

That's a two-sided sword: We actually shouldn't be setting realized =
true manually but once on machine init - in that case we wouldn't
strictly need a constant.

I pushed to get that central infrastructure in place to spare me/us the
repetitive realized = true setting, but Paolo shot it down, asking for a
full-fledged solver to make ordering guarantees.

> If there's a typo somewhere it will all fail at runtime
> in a hard to debug way, won't it?

It would. However, this is typically executed once on startup, so with
proper error handling we should notice this immediately. My qom-test
(that Anthony didn't take for 1.6 and I still need to respin) served to
test them, with focus on SysBusDevices.

You are cordially invited to add trivial qtests covering instantiation
of PCI/virtio devices you care about. :)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg