| Message ID | 1397828484-21771-2-git-send-email-batuzovk@ispras.ru |
|---|---|
| State | New |
| Headers | show |
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 <batuzovk@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. > @@ -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
Andreas Färber писал 2014-04-18 20:30: > 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 <batuzovk@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. > ACPI_PCIHP_PROP_BSEL is unsigned 32 bit integer, so it can not be negative. object_property_get_int returns int64_t, so even overflow can not cause negative values to appear. >> @@ -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
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 <batuzovk@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? > > @@ -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. Hmm int is 32 bit and object_property_get_int can return a 64 bit one. > The alternative would be to leave the wrapper around and just replace > the ..._get_qobject() with the ..._get_int() inside. Yes, I'd prefer that, and extra validation there too. > 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
On Sun, 20 Apr 2014 11:32:23 +0300 "Michael S. Tsirkin" <mst@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 <batuzovk@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. > > > > > @@ -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. > > Hmm int is 32 bit and object_property_get_int can return a 64 bit one. > > > The alternative would be to leave the wrapper around and just replace > > the ..._get_qobject() with the ..._get_int() inside. > > Yes, I'd prefer that, and extra validation there too. > > > 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 >
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" <mst@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 <batuzovk@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? > > > > > > > > @@ -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. > > > > Hmm int is 32 bit and object_property_get_int can return a 64 bit one. > > > > > The alternative would be to leave the wrapper around and just replace > > > the ..._get_qobject() with the ..._get_int() inside. > > > > Yes, I'd prefer that, and extra validation there too. > > > > > 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 > >
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" <mst@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 <batuzovk@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
On Tue, 22 Apr 2014 11:25:28 +0200 Andreas Färber <afaerber@suse.de> 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" <mst@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 <batuzovk@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. :) 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 >
On Tue, 22 Apr 2014, Igor Mammedov wrote: > On Tue, 22 Apr 2014 11:25:28 +0200 > Andreas Färber <afaerber@suse.de> 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" <mst@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 <batuzovk@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. :) > 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. Negative values for BSEL are also erroneous and in the end we will return negative values for errors from wrapper as well. Using Error * instead of return value here only protects us from sudden changes in object_property_get_int behaviour, which is not likely. On the other hand it may be interesting to distinguish two cases: * ACPI_PCIHP_PROP_BSEL is not set (correct, bus just does not support hotplug), * ACPI_PCIHP_PROP_BSEL is set but it's type or value is not correct (internal error, either incorrect hotplug support or properties names overlap). But to do this we'll need to return to the original wrapper (get QObject and then convert it to int) reimplementing object_property_get_int again. Parsing errors returned from object_property_get_int does not sound like a good idea to me because both errors are of the same class and differ only in error messages. Does this distinction worth having nearly duplicate code of object_property_get_int or not?
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; } } @@ -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");
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 <batuzovk@ispras.ru> --- hw/acpi/pcihp.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-)