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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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.
> 
> 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 <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)
> >  {
> 
>