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

[Kirill Batuzov]

On Tue, 22 Apr 2014, Igor Mammedov wrote:

> On Tue, 22 Apr 2014 11:25:28 +0200
> Andreas Färber <address@hidden> 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" <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. :)
> 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?

-- 
Kirill