From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41632) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUagD-0005lx-S3 for qemu-devel@nongnu.org; Tue, 02 Aug 2016 10:28:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUag9-0001vq-Gz for qemu-devel@nongnu.org; Tue, 02 Aug 2016 10:28:24 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:39098) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUag9-0001v7-8L for qemu-devel@nongnu.org; Tue, 02 Aug 2016 10:28:21 -0400 Date: Tue, 2 Aug 2016 10:28:18 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <2014655900.496159.1470148098168.JavaMail.zimbra@redhat.com> In-Reply-To: <20160802143649.52506ce6@nial.brq.redhat.com> References: <20160728111357.3349-1-marcandre.lureau@redhat.com> <20160802143649.52506ce6@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: marcandre lureau , qemu-devel@nongnu.org, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com Hi ----- Original Message ----- > On Thu, 28 Jul 2016 15:13:57 +0400 > marcandre.lureau@redhat.com wrote: >=20 > > From: Marc-Andr=C3=A9 Lureau > >=20 > > The property should own the allocated and unreferenced pointer. In case > > of error, it should also be freed. >=20 > 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: >=20 > object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL= , > - bus_bsel, NULL); > + bus_bsel, &error_abort); >=20 That's fine with me, but it will abort with /x86_64/qom/pc-i440fx-1.7 test.= Any idea why? > >=20 > > 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') > >=20 > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > --- > > hw/i386/acpi-build.c | 15 +++++++++++++-- > > include/qom/object.h | 4 ++++ > > qom/object.c | 9 +++++++++ > > 3 files changed, 26 insertions(+), 2 deletions(-) > >=20 > > 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); > > } > > =20 > > +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 chan= ged > > * to only assign to buses that support hotplug. > > */ > > @@ -432,13 +437,19 @@ static void *acpi_set_bsel(PCIBus *bus, void *opa= que) > > { > > unsigned *bsel_alloc =3D opaque; > > unsigned *bus_bsel; > > + Error *err =3D NULL; > > =20 > > if (qbus_is_hotpluggable(BUS(bus))) { > > bus_bsel =3D g_malloc(sizeof *bus_bsel); > > =20 > > *bus_bsel =3D (*bsel_alloc)++; > > - object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BS= EL, > > - 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); > > + } > > } > > =20 > > 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 *na= me, > > */ > > 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 *n= ame, > > + uint32_t *v, > > + ObjectPropertyRelease > > *release, > > + Error **errp); > > void object_class_property_add_uint32_ptr(ObjectClass *klass, const ch= ar > > *name, > > const uint32_t *v, Error > > **errp); > > =20 > > 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); > > } > > =20 > > +void object_property_add_uint32_ptr_release(Object *obj, const char *n= ame, > > + 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 ch= ar > > *name, > > const uint32_t *v, Error **e= rrp) > > { >=20 >=20