Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int

[Igor Mammedov]

On Tue, 22 Apr 2014 11:25:28 +0200
Andreas Färber <address@hidden> wrote:

> Am 22.04.2014 11:12, schrieb Michael S. Tsirkin:
> > On Tue, Apr 22, 2014 at 11:04:37AM +0200, Igor Mammedov wrote:
> >> On Sun, 20 Apr 2014 11:32:23 +0300
> >> "Michael S. Tsirkin" <address@hidden> wrote:
> >>
> >>> On Fri, Apr 18, 2014 at 06:30:37PM +0200, Andreas Färber wrote:
> >>>> Am 18.04.2014 15:41, schrieb Kirill Batuzov:
> >>>>> acpi_pcihp_get_bsel implements functionality of object_property_get_int 
> >>>>> for
> >>>>> specific property named ACPI_PCIHP_PROP_BSEL, but fails to decrement 
> >>>>> object's
> >>>>> reference counter properly. Replacing it with generic 
> >>>>> object_property_get_int
> >>>>> serves two purposes: reducing code duplication and fixing memory leak.
> >>>>>
> >>>>> Signed-off-by: Kirill Batuzov <address@hidden>
> >>>>> ---
> >>>>>  hw/acpi/pcihp.c |   23 ++++++-----------------
> >>>>>  1 file changed, 6 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >>>>> index f80c480..ff44aec 100644
> >>>>> --- a/hw/acpi/pcihp.c
> >>>>> +++ b/hw/acpi/pcihp.c
> >>>>> @@ -61,24 +61,11 @@ typedef struct AcpiPciHpFind {
> >>>>>      PCIBus *bus;
> >>>>>  } AcpiPciHpFind;
> >>>>>  
> >>>>> -static int acpi_pcihp_get_bsel(PCIBus *bus)
> >>>>> -{
> >>>>> -    QObject *o = object_property_get_qobject(OBJECT(bus),
> >>>>> -                                             ACPI_PCIHP_PROP_BSEL, 
> >>>>> NULL);
> >>>>> -    int64_t bsel = -1;
> >>>>> -    if (o) {
> >>>>> -        bsel = qint_get_int(qobject_to_qint(o));
> >>>>> -    }
> >>>>> -    if (bsel < 0) {
> >>>>> -        return -1;
> >>>>> -    }
> >>>>> -    return bsel;
> >>>>> -}
> >>>>> -
> >>>>>  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> >>>>>  {
> >>>>>      AcpiPciHpFind *find = opaque;
> >>>>> -    if (find->bsel == acpi_pcihp_get_bsel(bus)) {
> >>>>> +    if (find->bsel == object_property_get_int(OBJECT(bus),
> >>>>> +                                              ACPI_PCIHP_PROP_BSEL, 
> >>>>> NULL)) {
> >>>>>          find->bus = bus;
> >>>>>      }
> >>>>>  }
> >>>>
> >>>> I note that the wrapper function was changing negative values up to be
> >>>> -1, which is getting dropped here. Not sure if it matters.
> >>>
> >>> I think this was to ensure that we don't get an overflow.
> >>> I'm not sure why didn't I validate against ACPI_PCIHP_MAX_HOTPLUG_BUS
> >>> too.
> >>> How about making acpi_pcihp_get_bsel call object_property_get_int
> >>> and validate that value is between 0 and ACPI_PCIHP_MAX_HOTPLUG_BUS?
> >> We need acpi_pcihp_get_bsel() since not every bus might have
> >> ACPI_PCIHP_PROP_BSEL, so blindly replacing it with 
> >> object_property_get_int()
> >> would be wrong.
> > 
> > object_property_get_int returns -1 on failure or am I misreading the code?
> 
> Correct, I had checked that before my reply.
> 
> But if we keep the helper function around and check for Error ** there,
> it becomes irrelevant. :)
Yep, that's my point. -1 for object_property_get_int() is also a valid value,
so checking errp would be more robust instead of depending on returned value.

> 
> Cheers,
> Andreas
>