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" <m...@redhat.com> 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 <batuz...@ispras.ru> >>>>> --- >>>>> 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