Hi
----- Original Message -----
> On Thu, 28 Jul 2016 15:13:57 +0400
> marcandre.lur...@redhat.com wrote:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > The property should own the allocated and unreferenced pointer. In case
> > of error, it should also be freed.
>
> acpi_setup() -> acpi_set_pci_info() -> acpi_set_bsel()
> is called only once at machine done time
> so if error happens it would be better to handle it instead of
> just freeing bus_bsel pointer.
> Since it's error path at machine startup time, typical reaction
> to an unexpected error would be abort(), so following change
> should be sufficient:
>
> object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> - bus_bsel, NULL);
> + bus_bsel, &error_abort);
>
That's fine with me, but it will abort with /x86_64/qom/pc-i440fx-1.7 test. Any
idea why?
> >
> > RFC, because this patch triggers:
> > /x86_64/qom/pc-i440fx-1.7:
> > qemu-system-x86_64: attempt to add duplicate property 'acpi-pcihp-bsel' to
> > object (type 'PCI')
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > hw/i386/acpi-build.c | 15 +++++++++++++--
> > include/qom/object.h | 4 ++++
> > qom/object.c | 9 +++++++++
> > 3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 017bb51..2012007 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -425,6 +425,11 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> > PCMachineState *pcms)
> > table_data->len - madt_start, 1, NULL, NULL);
> > }
> >
> > +static void bsel_release(Object *obj, const char *name, void *opaque)
> > +{
> > + g_free(opaque);
> > +}
> > +
> > /* Assign BSEL property to all buses. In the future, this can be changed
> > * to only assign to buses that support hotplug.
> > */
> > @@ -432,13 +437,19 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> > {
> > unsigned *bsel_alloc = opaque;
> > unsigned *bus_bsel;
> > + Error *err = NULL;
> >
> > if (qbus_is_hotpluggable(BUS(bus))) {
> > bus_bsel = g_malloc(sizeof *bus_bsel);
> >
> > *bus_bsel = (*bsel_alloc)++;
> > - object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > - bus_bsel, NULL);
> > + object_property_add_uint32_ptr_release(OBJECT(bus),
> > + ACPI_PCIHP_PROP_BSEL,
> > + bus_bsel, bsel_release,
> > &err);
> > + if (err) {
> > + g_free(bus_bsel);
> > + error_report_err(err);
> > + }
> > }
> >
> > return bsel_alloc;
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 5ecc2d1..41c1051 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1488,6 +1488,10 @@ void
> > object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name,
> > */
> > void object_property_add_uint32_ptr(Object *obj, const char *name,
> > const uint32_t *v, Error **errp);
> > +void object_property_add_uint32_ptr_release(Object *obj, const char *name,
> > + uint32_t *v,
> > + ObjectPropertyRelease
> > *release,
> > + Error **errp);
> > void object_class_property_add_uint32_ptr(ObjectClass *klass, const char
> > *name,
> > const uint32_t *v, Error
> > **errp);
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index 8166b7d..1635f57 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -2157,6 +2157,15 @@ void object_property_add_uint32_ptr(Object *obj,
> > const char *name,
> > NULL, NULL, (void *)v, errp);
> > }
> >
> > +void object_property_add_uint32_ptr_release(Object *obj, const char *name,
> > + uint32_t *v,
> > + ObjectPropertyRelease
> > *release,
> > + Error **errp)
> > +{
> > + object_property_add(obj, name, "uint32", property_get_uint32_ptr,
> > + NULL, release, (void *)v, errp);
> > +}
> > +
> > void object_class_property_add_uint32_ptr(ObjectClass *klass, const char
> > *name,
> > const uint32_t *v, Error **errp)
> > {
>
>
