* [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel
@ 2016-07-28 11:13 marcandre.lureau
2016-07-28 11:29 ` Igor Mammedov
2016-08-02 12:36 ` Igor Mammedov
0 siblings, 2 replies; 7+ messages in thread
From: marcandre.lureau @ 2016-07-28 11:13 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, imammedo, rth, ehabkost, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
The property should own the allocated and unreferenced pointer. In case
of error, it should also be freed.
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.lureau@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)
{
--
2.9.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel
2016-07-28 11:13 [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel marcandre.lureau
@ 2016-07-28 11:29 ` Igor Mammedov
2016-07-28 13:43 ` Marc-André Lureau
2016-08-02 12:36 ` Igor Mammedov
1 sibling, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2016-07-28 11:29 UTC (permalink / raw)
To: marcandre.lureau; +Cc: qemu-devel, mst, rth, ehabkost
On Thu, 28 Jul 2016 15:13:57 +0400
marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The property should own the allocated and unreferenced pointer. In case
> of error, it should also be freed.
I wonder, what use case triggers above error
>
> 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.lureau@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)
> {
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel
2016-07-28 11:29 ` Igor Mammedov
@ 2016-07-28 13:43 ` Marc-André Lureau
2016-07-29 6:31 ` Igor Mammedov
0 siblings, 1 reply; 7+ messages in thread
From: Marc-André Lureau @ 2016-07-28 13:43 UTC (permalink / raw)
To: Igor Mammedov
Cc: Richard Henderson, QEMU, Eduardo Habkost, Michael S. Tsirkin
Hi
On Thu, Jul 28, 2016 at 3:29 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 28 Jul 2016 15:13:57 +0400
> marcandre.lureau@redhat.com wrote:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> The property should own the allocated and unreferenced pointer. In case
>> of error, it should also be freed.
> I wonder, what use case triggers above error
>
>
See right below in commit message:
/x86_64/qom/pc-i440fx-1.7: qemu-system-x86_64: attempt to add
duplicate property 'acpi-pcihp-bsel' to object (type 'PCI')
>>
>> 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.lureau@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)
>> {
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel
2016-07-28 13:43 ` Marc-André Lureau
@ 2016-07-29 6:31 ` Igor Mammedov
2016-07-29 7:30 ` Marc-André Lureau
0 siblings, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2016-07-29 6:31 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Richard Henderson, QEMU, Eduardo Habkost, Michael S. Tsirkin
On Thu, 28 Jul 2016 17:43:37 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Thu, Jul 28, 2016 at 3:29 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 28 Jul 2016 15:13:57 +0400
> > marcandre.lureau@redhat.com wrote:
> >
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> The property should own the allocated and unreferenced pointer. In case
> >> of error, it should also be freed.
> > I wonder, what use case triggers above error
> >
> >
>
> See right below in commit message:
>
> /x86_64/qom/pc-i440fx-1.7: qemu-system-x86_64: attempt to add
> duplicate property 'acpi-pcihp-bsel' to object (type 'PCI')
Maybe I've should have asked in another way,
How do you reproduce it?
>
>
> >>
> >> 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.lureau@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)
> >> {
> >
> >
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel
2016-07-29 6:31 ` Igor Mammedov
@ 2016-07-29 7:30 ` Marc-André Lureau
0 siblings, 0 replies; 7+ messages in thread
From: Marc-André Lureau @ 2016-07-29 7:30 UTC (permalink / raw)
To: Igor Mammedov
Cc: Richard Henderson, QEMU, Eduardo Habkost, Michael S. Tsirkin
Hi
On Fri, Jul 29, 2016 at 10:31 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 28 Jul 2016 17:43:37 +0400
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, Jul 28, 2016 at 3:29 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> > On Thu, 28 Jul 2016 15:13:57 +0400
>> > marcandre.lureau@redhat.com wrote:
>> >
>> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >>
>> >> The property should own the allocated and unreferenced pointer. In case
>> >> of error, it should also be freed.
>> > I wonder, what use case triggers above error
>> >
>> >
>>
>> See right below in commit message:
>>
>> /x86_64/qom/pc-i440fx-1.7: qemu-system-x86_64: attempt to add
>> duplicate property 'acpi-pcihp-bsel' to object (type 'PCI')
> Maybe I've should have asked in another way,
> How do you reproduce it?
It's a qtest error (with the error reporting added here):
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/qom-test -p
/x86_64/qom/pc-i440fx-1.7
This is also triggered simply by running:
x86_64-softmmu/qemu-system-x86_64 -machine pc-i440fx-1.7
>
>>
>>
>> >>
>> >> 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.lureau@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)
>> >> {
>> >
>> >
>>
>>
>>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel
2016-07-28 11:13 [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel marcandre.lureau
2016-07-28 11:29 ` Igor Mammedov
@ 2016-08-02 12:36 ` Igor Mammedov
2016-08-02 14:28 ` Marc-André Lureau
1 sibling, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2016-08-02 12:36 UTC (permalink / raw)
To: marcandre.lureau; +Cc: qemu-devel, mst, rth, ehabkost
On Thu, 28 Jul 2016 15:13:57 +0400
marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@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);
>
> 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.lureau@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)
> {
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel
2016-08-02 12:36 ` Igor Mammedov
@ 2016-08-02 14:28 ` Marc-André Lureau
0 siblings, 0 replies; 7+ messages in thread
From: Marc-André Lureau @ 2016-08-02 14:28 UTC (permalink / raw)
To: Igor Mammedov; +Cc: marcandre lureau, qemu-devel, mst, rth, ehabkost
Hi
----- Original Message -----
> On Thu, 28 Jul 2016 15:13:57 +0400
> marcandre.lureau@redhat.com wrote:
>
> > From: Marc-André Lureau <marcandre.lureau@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.lureau@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)
> > {
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-02 14:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-28 11:13 [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel marcandre.lureau
2016-07-28 11:29 ` Igor Mammedov
2016-07-28 13:43 ` Marc-André Lureau
2016-07-29 6:31 ` Igor Mammedov
2016-07-29 7:30 ` Marc-André Lureau
2016-08-02 12:36 ` Igor Mammedov
2016-08-02 14:28 ` Marc-André Lureau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.