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

[Andreas Färber]

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.

> @@ -185,7 +172,8 @@ void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq 
> irq, AcpiPciHpState *s,
>  {
>      PCIDevice *pdev = PCI_DEVICE(dev);
>      int slot = PCI_SLOT(pdev->devfn);
> -    int bsel = acpi_pcihp_get_bsel(pdev->bus);
> +    int bsel = object_property_get_int(OBJECT(pdev->bus),
> +                                       ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel < 0) {
>          error_setg(errp, "Unsupported bus. Bus doesn't have property '"
>                     ACPI_PCIHP_PROP_BSEL "' set");
> @@ -210,7 +198,8 @@ void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq 
> irq, AcpiPciHpState *s,
>  {
>      PCIDevice *pdev = PCI_DEVICE(dev);
>      int slot = PCI_SLOT(pdev->devfn);
> -    int bsel = acpi_pcihp_get_bsel(pdev->bus);
> +    int bsel = object_property_get_int(OBJECT(pdev->bus),
> +                                       ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel < 0) {
>          error_setg(errp, "Unsupported bus. Bus doesn't have property '"
>                     ACPI_PCIHP_PROP_BSEL "' set");

These ones seem to just check for < 0, so look okay from reading the
patch. CC'ing mst.

The alternative would be to leave the wrapper around and just replace
the ..._get_qobject() with the ..._get_int() inside.

Regards,
Andreas

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