From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUYwM-0001kW-Hf for qemu-devel@nongnu.org; Tue, 02 Aug 2016 08:36:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUYwH-0008JJ-GT for qemu-devel@nongnu.org; Tue, 02 Aug 2016 08:36:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40532) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUYwH-0008JE-8D for qemu-devel@nongnu.org; Tue, 02 Aug 2016 08:36:53 -0400 Date: Tue, 2 Aug 2016 14:36:49 +0200 From: Igor Mammedov Message-ID: <20160802143649.52506ce6@nial.brq.redhat.com> In-Reply-To: <20160728111357.3349-1-marcandre.lureau@redhat.com> References: <20160728111357.3349-1-marcandre.lureau@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: marcandre.lureau@redhat.com Cc: qemu-devel@nongnu.org, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com On Thu, 28 Jul 2016 15:13:57 +0400 marcandre.lureau@redhat.com wrote: > 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. 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); >=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' t= o 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, P= CMachineState *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 changed > * to only assign to buses that support hotplug. > */ > @@ -432,13 +437,19 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaqu= e) > { > 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_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); > + } > } > =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(ObjectCl= ass *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 *nam= e, > + uint32_t *v, > + ObjectPropertyRelease *relea= se, > + Error **errp); > void object_class_property_add_uint32_ptr(ObjectClass *klass, const char= *name, > const uint32_t *v, Error **err= p); > =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, c= onst char *name, > NULL, NULL, (void *)v, errp); > } > =20 > +void object_property_add_uint32_ptr_release(Object *obj, const char *nam= e, > + uint32_t *v, > + ObjectPropertyRelease *relea= se, > + 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 **err= p) > {