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

[Andreas Färber]

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. :)

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg