[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic ob
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int |
Date: |
Tue, 22 Apr 2014 11:04:37 +0200 |
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.
>
>
> > > @@ -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
>
- Re: [Qemu-devel] [PATCH 3/4] graphic_console_init: do not receive unneeded error descriptions, (continued)
[Qemu-devel] [PATCH 4/4] PortioList: fix PortioList uses so they do not leak memory, Kirill Batuzov, 2014/04/18
[Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int, Kirill Batuzov, 2014/04/18
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int, Andreas Färber, 2014/04/18
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get _bsel with generic object_property_get_int, Kirill Batuzov, 2014/04/18
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int, Michael S. Tsirkin, 2014/04/20
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int,
Igor Mammedov <=
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int, Michael S. Tsirkin, 2014/04/22
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int, Andreas Färber, 2014/04/22
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int, Igor Mammedov, 2014/04/22
- Re: [Qemu-devel] [PATCH 1/4] Replace acpi_pcihp_get_bsel with generic object_property_get_int, Kirill Batuzov, 2014/04/22
[Qemu-devel] [PATCH 2/4] acpi-build: properly decrement objects' reference counters, Kirill Batuzov, 2014/04/18
Re: [Qemu-devel] [PATCH 0/4] Fix memory leaks in QEMU, Peter Maydell, 2014/04/18
Re: [Qemu-devel] [PATCH 0/4] Fix memory leaks in QEMU, Markus Armbruster, 2014/04/22