[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properti
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties |
Date: |
Mon, 16 Sep 2013 18:48:51 +0300 |
On Mon, Sep 16, 2013 at 05:24:46PM +0200, Andreas Färber wrote:
> 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
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
Even then: it will be at best
"5. Do it right or it will always break at runtime."
We need to switch to APIs at
"9. The compiler/linker won't let you get it wrong."
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
- [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Michael S. Tsirkin, 2013/09/15
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Peter Maydell, 2013/09/15
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Andreas Färber, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Michael S. Tsirkin, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Andreas Färber, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Paolo Bonzini, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Paolo Bonzini, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Michael S. Tsirkin, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Paolo Bonzini, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Michael S. Tsirkin, 2013/09/16
- Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Paolo Bonzini, 2013/09/16
Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties, Michael S. Tsirkin, 2013/09/16