Re: [Qemu-devel] [PATCH] RFC: pci-bus: add property ownership on bsel

[Marc-André Lureau]

Hi

On Fri, Jul 29, 2016 at 10:31 AM, Igor Mammedov <address@hidden> wrote:
> On Thu, 28 Jul 2016 17:43:37 +0400
> Marc-André Lureau <address@hidden> wrote:
>
>> Hi
>>
>> On Thu, Jul 28, 2016 at 3:29 PM, Igor Mammedov <address@hidden> wrote:
>> > On Thu, 28 Jul 2016 15:13:57 +0400
>> > address@hidden wrote:
>> >
>> >> From: Marc-André Lureau <address@hidden>
>> >>
>> >> 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 <address@hidden>
>> >> ---
>> >>  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