[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 15/17] Use uint property getter/setter where app
|
From: |
Markus Armbruster |
|
Subject: |
Re: [Qemu-devel] [PATCH 15/17] Use uint property getter/setter where appropriate |
|
Date: |
Thu, 18 May 2017 17:20:28 +0200 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> All those property usages are associated with unsigned integers, so use
> appropriate getter/setter.
"Usages"? I think this is a question of whether the property value is
signed or unsigned. I guess "those properties are" would work.
How did you find the ones that need changing?
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
> include/hw/isa/isa.h | 2 +-
> hw/acpi/memory_hotplug.c | 10 ++++----
> hw/acpi/nvdimm.c | 10 ++++----
> hw/acpi/pcihp.c | 6 ++---
> hw/arm/aspeed.c | 4 ++--
> hw/arm/bcm2835_peripherals.c | 9 ++++----
> hw/arm/raspi.c | 4 ++--
> hw/core/platform-bus.c | 2 +-
> hw/i386/acpi-build.c | 55
> ++++++++++++++++++++++----------------------
> hw/i386/pc.c | 6 ++---
> hw/i386/xen/xen-hvm.c | 6 ++---
> hw/intc/arm_gicv3_common.c | 2 +-
> hw/mem/pc-dimm.c | 5 ++--
> hw/misc/auxbus.c | 2 +-
> hw/misc/pvpanic.c | 2 +-
> hw/ppc/pnv_core.c | 2 +-
> hw/ppc/spapr.c | 8 ++++---
> numa.c | 6 ++---
> target/i386/cpu.c | 4 ++--
> ui/console.c | 4 ++--
> 20 files changed, 77 insertions(+), 72 deletions(-)
>
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index c2fdd70cdc..95593408ef 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -29,7 +29,7 @@ static inline uint16_t applesmc_port(void)
> Object *obj = object_resolve_path_type("", TYPE_APPLE_SMC, NULL);
>
> if (obj) {
> - return object_property_get_int(obj, APPLESMC_PROP_IO_BASE, NULL);
> + return object_property_get_uint(obj, APPLESMC_PROP_IO_BASE, NULL);
> }
> return 0;
> }
The property is defined with DEFINE_PROP_UINT32(). Okay.
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 210073d283..db3c89ceab 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -83,11 +83,12 @@ static uint64_t acpi_memory_hotplug_read(void *opaque,
> hwaddr addr,
> o = OBJECT(mdev->dimm);
> switch (addr) {
> case 0x0: /* Lo part of phys address where DIMM is mapped */
> - val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) : 0;
> + val = o ? object_property_get_uint(o, PC_DIMM_ADDR_PROP, NULL) : 0;
> trace_mhp_acpi_read_addr_lo(mem_st->selector, val);
> break;
> case 0x4: /* Hi part of phys address where DIMM is mapped */
> - val = o ? object_property_get_int(o, PC_DIMM_ADDR_PROP, NULL) >> 32
> : 0;
> + val =
> + o ? object_property_get_uint(o, PC_DIMM_ADDR_PROP, NULL) >> 32 :
> 0;
> trace_mhp_acpi_read_addr_hi(mem_st->selector, val);
> break;
> case 0x8: /* Lo part of DIMM size */
TYPE_PC_DIMM's property PC_DIMM_ADDR_PROP is defined with
DEFINE_PROP_UINT64(). Okay.
> @@ -95,11 +96,12 @@ static uint64_t acpi_memory_hotplug_read(void *opaque,
> hwaddr addr,
> trace_mhp_acpi_read_size_lo(mem_st->selector, val);
> break;
> case 0xc: /* Hi part of DIMM size */
> - val = o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32
> : 0;
> + val =
> + o ? object_property_get_int(o, PC_DIMM_SIZE_PROP, NULL) >> 32 :
> 0;
pc_dimm_get_size() uses visit_type_int(). Does that need a matching
update?
> trace_mhp_acpi_read_size_hi(mem_st->selector, val);
> break;
> case 0x10: /* node proximity for _PXM method */
> - val = o ? object_property_get_int(o, PC_DIMM_NODE_PROP, NULL) : 0;
> + val = o ? object_property_get_uint(o, PC_DIMM_NODE_PROP, NULL) : 0;
> trace_mhp_acpi_read_pxm(mem_st->selector, val);
> break;
> case 0x14: /* pack and return is_* fields */
TYPE_PC_DIMM's property PC_DIMM_NODE_PROP is defined with
DEFINE_PROP_UINT32(). Okay.
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 8e7d6ec034..e57027149d 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -236,14 +236,14 @@ static void
> nvdimm_build_structure_spa(GArray *structures, DeviceState *dev)
> {
> NvdimmNfitSpa *nfit_spa;
> - uint64_t addr = object_property_get_int(OBJECT(dev), PC_DIMM_ADDR_PROP,
> - NULL);
> + uint64_t addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
> + NULL);
> uint64_t size = object_property_get_int(OBJECT(dev), PC_DIMM_SIZE_PROP,
> NULL);
Here, you leave PC_DIMM_SIZE_PROP alone. The code gets it signed and
assigns it to unsigned. Needs cleanup.
> - uint32_t node = object_property_get_int(OBJECT(dev), PC_DIMM_NODE_PROP,
> - NULL);
> + uint32_t node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
> + NULL);
> int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> - NULL);
> + NULL);
>
> nfit_spa = acpi_data_push(structures, sizeof(*nfit_spa));
>
More of the same.
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 3a531a4416..c420a388ea 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -62,10 +62,10 @@ typedef struct AcpiPciHpFind {
> static int acpi_pcihp_get_bsel(PCIBus *bus)
> {
> Error *local_err = NULL;
> - int64_t bsel = object_property_get_int(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> - &local_err);
> + uint64_t bsel = object_property_get_uint(OBJECT(bus),
> ACPI_PCIHP_PROP_BSEL,
> + &local_err);
>
> - if (local_err || bsel < 0 || bsel >= ACPI_PCIHP_MAX_HOTPLUG_BUS) {
> + if (local_err || bsel >= ACPI_PCIHP_MAX_HOTPLUG_BUS) {
> if (local_err) {
> error_free(local_err);
> }
Haven't I seen ACPI_PCIHP_PROP_BSEL before? Yes, in PATCH 13. I think
splitting along subsystems rather than functions changed would be easier
to review, because the what the reviewer needs to understand is not so
much the functions but the underlying properties.
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 283c038814..4af422909f 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -187,8 +187,8 @@ static void aspeed_board_init(MachineState *machine,
> * Allocate RAM after the memory controller has checked the size
> * was valid. If not, a default value is used.
> */
> - ram_size = object_property_get_int(OBJECT(&bmc->soc), "ram-size",
> - &error_abort);
> + ram_size = object_property_get_uint(OBJECT(&bmc->soc), "ram-size",
> + &error_abort);
The assignment to global ram_size looks nasty, but that particular
nastiness is outside this patch's scope.
If I understand the aspeed code correctly, then this property is an
alias for device TYPE_ASPEED_SDMC's property "ram-size", which is
defined with DEFINE_PROP_UINT64(). Okay.
However, a few lines further up, we have:
object_property_set_int(OBJECT(&bmc->soc), ram_size, "ram-size",
&error_abort);
The two should be kept consistent.
Adds urgency to my question how you found the ones that need changing.
>
> memory_region_allocate_system_memory(&bmc->ram, NULL, "ram", ram_size);
> memory_region_add_subregion(get_system_memory(), sc->info->sdram_base,
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index 369ef1e3bd..b168c6f83e 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -126,7 +126,7 @@ static void bcm2835_peripherals_realize(DeviceState *dev,
> Error **errp)
> Object *obj;
> MemoryRegion *ram;
> Error *err = NULL;
> - uint32_t ram_size, vcram_size;
> + uint64_t ram_size, vcram_size;
> int n;
>
> obj = object_property_get_link(OBJECT(dev), "ram", &err);
> @@ -208,15 +208,14 @@ static void bcm2835_peripherals_realize(DeviceState
> *dev, Error **errp)
> INTERRUPT_ARM_MAILBOX));
>
> /* Framebuffer */
> - vcram_size = (uint32_t)object_property_get_int(OBJECT(s), "vcram-size",
> - &err);
> + vcram_size = object_property_get_uint(OBJECT(s), "vcram-size", &err);
This one appears to be an alias of TYPE_BCM2835_FB's property
"vcram-size", which is defined with DEFINE_PROP_UINT32(). Okay.
> if (err) {
> error_propagate(errp, err);
> return;
> }
>
> - object_property_set_int(OBJECT(&s->fb), ram_size - vcram_size,
> - "vcram-base", &err);
> + object_property_set_uint(OBJECT(&s->fb), ram_size - vcram_size,
> + "vcram-base", &err);
> if (err) {
> error_propagate(errp, err);
> return;
Defined with DEFINE_PROP_UINT32(). Okay.
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 2b295f14c4..32cdc98c6d 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -153,8 +153,8 @@ static void raspi2_init(MachineState *machine)
> qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> object_property_set_bool(OBJECT(carddev), true, "realized",
> &error_fatal);
>
> - vcram_size = object_property_get_int(OBJECT(&s->soc), "vcram-size",
> - &error_abort);
> + vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
> + &error_abort);
> setup_boot(machine, 2, machine->ram_size - vcram_size);
> }
Same property as above. Okay.
>
> diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
> index 329ac670c0..33d32fbf22 100644
> --- a/hw/core/platform-bus.c
> +++ b/hw/core/platform-bus.c
> @@ -71,7 +71,7 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus,
> SysBusDevice *sbdev,
> return -1;
> }
>
> - return object_property_get_int(OBJECT(sbdev_mr), "addr", NULL);
> + return object_property_get_uint(OBJECT(sbdev_mr), "addr", NULL);
> }
I figure this is TYPE_MEMORY_REGION's property. Its getter
memory_region_get_addr() uses visit_type_uint64(). Okay.
>
> static void platform_bus_count_irqs(SysBusDevice *sbdev, void *opaque)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 27ad420914..76d27ff024 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -137,9 +137,9 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> obj = piix;
> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> pm->pcihp_io_base =
> - object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> + object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> pm->pcihp_io_len =
> - object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> + object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> }
> if (lpc) {
> obj = lpc;
These appear to be defined with object_property_add_uint16_ptr(). Okay,
I think.
> @@ -171,20 +171,21 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> qobject_decref(o);
>
> /* Fill in mandatory properties */
> - pm->sci_int = object_property_get_int(obj, ACPI_PM_PROP_SCI_INT, NULL);
> -
> - pm->acpi_enable_cmd = object_property_get_int(obj,
> -
> ACPI_PM_PROP_ACPI_ENABLE_CMD,
> - NULL);
> - pm->acpi_disable_cmd = object_property_get_int(obj,
> -
> ACPI_PM_PROP_ACPI_DISABLE_CMD,
> - NULL);
> - pm->io_base = object_property_get_int(obj, ACPI_PM_PROP_PM_IO_BASE,
> - NULL);
> - pm->gpe0_blk = object_property_get_int(obj, ACPI_PM_PROP_GPE0_BLK,
> + pm->sci_int = object_property_get_uint(obj, ACPI_PM_PROP_SCI_INT, NULL);
> +
> + pm->acpi_enable_cmd = object_property_get_uint(obj,
> +
> ACPI_PM_PROP_ACPI_ENABLE_CMD,
> + NULL);
> + pm->acpi_disable_cmd =
> + object_property_get_uint(obj,
> + ACPI_PM_PROP_ACPI_DISABLE_CMD,
> + NULL);
> + pm->io_base = object_property_get_uint(obj, ACPI_PM_PROP_PM_IO_BASE,
> NULL);
> - pm->gpe0_blk_len = object_property_get_int(obj,
> ACPI_PM_PROP_GPE0_BLK_LEN,
> - NULL);
> + pm->gpe0_blk = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK,
> + NULL);
> + pm->gpe0_blk_len = object_property_get_uint(obj,
> ACPI_PM_PROP_GPE0_BLK_LEN,
> + NULL);
> pm->pcihp_bridge_en =
> object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> NULL);
PIIX4: piix4_pm_add_propeties() defines these with
object_property_add_uint*_ptr().
Q35: ich9_lpc_add_properties() and ich9_pm_add_properties() define them
similarly, except for ACPI_PM_PROP_GPE0_BLK(). That one's getter
ich9_pm_get_gpe0_blk() uses visit_type_uint32().
Okay.
> @@ -237,19 +238,19 @@ static void acpi_get_pci_holes(Range *hole, Range
> *hole64)
> g_assert(pci_host);
>
> range_set_bounds1(hole,
> - object_property_get_int(pci_host,
> - PCI_HOST_PROP_PCI_HOLE_START,
> - NULL),
> - object_property_get_int(pci_host,
> - PCI_HOST_PROP_PCI_HOLE_END,
> - NULL));
> + object_property_get_uint(pci_host,
> + PCI_HOST_PROP_PCI_HOLE_START,
> + NULL),
> + object_property_get_uint(pci_host,
> + PCI_HOST_PROP_PCI_HOLE_END,
> + NULL));
> range_set_bounds1(hole64,
> - object_property_get_int(pci_host,
> - PCI_HOST_PROP_PCI_HOLE64_START,
> - NULL),
> - object_property_get_int(pci_host,
> - PCI_HOST_PROP_PCI_HOLE64_END,
> - NULL));
> + object_property_get_uint(pci_host,
> +
> PCI_HOST_PROP_PCI_HOLE64_START,
> + NULL),
> + object_property_get_uint(pci_host,
> + PCI_HOST_PROP_PCI_HOLE64_END,
> + NULL));
> }
>
Deja vu again, this time PATCH 11. Okay.
> #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is APM_CNT_IOPORT */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3b372a18f..8dc4507aa5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -347,7 +347,7 @@ static int check_fdc(Object *obj, void *opaque)
> return 0;
> }
>
> - iobase = object_property_get_int(obj, "iobase", &local_err);
> + iobase = object_property_get_uint(obj, "iobase", &local_err);
> if (local_err || iobase != 0x3f0) {
> error_free(local_err);
> return 0;
TYPE_ISA_FDC's property "iobase" is defined with DEFINE_PROP_UINT32().
Okay.
> @@ -1100,7 +1100,7 @@ static void pc_new_cpu(const char *typename, int64_t
> apic_id, Error **errp)
>
> cpu = object_new(typename);
>
> - object_property_set_int(cpu, apic_id, "apic-id", &local_err);
> + object_property_set_uint(cpu, apic_id, "apic-id", &local_err);
> object_property_set_bool(cpu, true, "realized", &local_err);
>
TYPE_X86_CPU's property "apic-id" is defined with DEFINE_PROP_UINT32().
Okay.
> object_unref(cpu);
> @@ -1560,7 +1560,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq
> *gsi,
> * and earlier, use IRQ2 for compat. Otherwise, use IRQ16~23,
> * IRQ8 and IRQ2.
> */
> - uint8_t compat = object_property_get_int(OBJECT(hpet),
> + uint8_t compat = object_property_get_uint(OBJECT(hpet),
> HPET_INTCAP, NULL);
> if (!compat) {
> qdev_prop_set_uint32(hpet, HPET_INTCAP, hpet_irqs);
TYPE_HPET's property HPET_INTCAP is defined with DEFINE_PROP_UINT32().
Okay.
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index b1c05ffb86..cec259f82d 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -183,9 +183,9 @@ static void xen_ram_init(PCMachineState *pcms,
> {
> MemoryRegion *sysmem = get_system_memory();
> ram_addr_t block_len;
> - uint64_t user_lowmem = object_property_get_int(qdev_get_machine(),
> -
> PC_MACHINE_MAX_RAM_BELOW_4G,
> - &error_abort);
> + uint64_t user_lowmem = object_property_get_uint(qdev_get_machine(),
> +
> PC_MACHINE_MAX_RAM_BELOW_4G,
> + &error_abort);
>
> /* Handle the machine opt max-ram-below-4g. It is basically doing
> * min(xen limit, user limit).
TYPE_PC_MACHINE's property PC_MACHINE_MAX_RAM_BELOW_4G's getter and
setter pc_machine_get_max_ram_below_4g() and
pc_machine_set_max_ram_below_4g() use visit_type_size(). Okay.
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index c6493d6c07..e2064cd8c5 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -267,7 +267,7 @@ static void arm_gicv3_common_realize(DeviceState *dev,
> Error **errp)
> * VLPIS == 0 (virtual LPIs not supported)
> * PLPIS == 0 (physical LPIs not supported)
> */
> - cpu_affid = object_property_get_int(OBJECT(cpu), "mp-affinity",
> NULL);
> + cpu_affid = object_property_get_uint(OBJECT(cpu), "mp-affinity",
> NULL);
> last = (i == s->num_cpu - 1);
>
> /* The CPU mp-affinity property is in MPIDR register format; squash
TYPE_ARM_CPU's property "mp-affinity" is defined with
DEFINE_PROP_UINT64(). Okay.
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 9e8dab0e89..f6def8c239 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -46,7 +46,8 @@ void pc_dimm_memory_plug(DeviceState *dev,
> MemoryHotplugState *hpms,
> uint64_t existing_dimms_capacity = 0;
> uint64_t addr;
>
> - addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> + addr = object_property_get_uint(OBJECT(dimm),
> + PC_DIMM_ADDR_PROP, &local_err);
> if (local_err) {
> goto out;
> }
Discussed above.
> @@ -73,7 +74,7 @@ void pc_dimm_memory_plug(DeviceState *dev,
> MemoryHotplugState *hpms,
> goto out;
> }
>
> - object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
> &local_err);
> + object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
> &local_err);
> if (local_err) {
> goto out;
> }
Same.
> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
> index e4a7ba41de..8a90ddda84 100644
> --- a/hw/misc/auxbus.c
> +++ b/hw/misc/auxbus.c
> @@ -244,7 +244,7 @@ static void aux_slave_dev_print(Monitor *mon, DeviceState
> *dev, int indent)
>
> monitor_printf(mon, "%*smemory " TARGET_FMT_plx "/" TARGET_FMT_plx "\n",
> indent, "",
> - object_property_get_int(OBJECT(s->mmio), "addr", NULL),
> + object_property_get_uint(OBJECT(s->mmio), "addr", NULL),
> memory_region_size(s->mmio));
> }
>
I figure this is again TYPE_MEMORY_REGION's property.
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index 57da7f2199..2b1e9a6450 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -111,7 +111,7 @@ uint16_t pvpanic_port(void)
> if (!o) {
> return 0;
> }
> - return object_property_get_int(o, PVPANIC_IOPORT_PROP, NULL);
> + return object_property_get_uint(o, PVPANIC_IOPORT_PROP, NULL);
> }
>
TYPE_ISA_PVPANIC_DEVICE's property PVPANIC_IOPORT_PROP is defined with
DEFINE_PROP_UINT16(). Okay.
> static Property pvpanic_isa_properties[] = {
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 1b7ec70f03..142bad1e57 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -51,7 +51,7 @@ static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
> int thread_index = 0; /* TODO: TCG supports only one thread */
> ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
>
> - core_pir = object_property_get_int(OBJECT(cpu), "core-pir",
> &error_abort);
> + core_pir = object_property_get_uint(OBJECT(cpu), "core-pir",
> &error_abort);
>
> /*
> * The PIR of a thread is the core PIR + the thread index. We will
This seems to be an alias of TYPE_PNV_CORE's property "pir", which is
defined with DEFINE_PROP_UINT32(). Okay.
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 80d12d005c..9b9a4e8817 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2582,7 +2582,8 @@ static void spapr_memory_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> goto out;
> }
>
> - addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> + addr = object_property_get_uint(OBJECT(dimm),
> + PC_DIMM_ADDR_PROP, &local_err);
> if (local_err) {
> pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
> goto out;
Discussed above.
> @@ -2670,7 +2671,8 @@ static void spapr_memory_unplug_request(HotplugHandler
> *hotplug_dev,
> uint64_t size = memory_region_size(mr);
> uint64_t addr;
>
> - addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> &local_err);
> + addr = object_property_get_uint(OBJECT(dimm),
> + PC_DIMM_ADDR_PROP, &local_err);
> if (local_err) {
> goto out;
> }
Same.
> @@ -2878,7 +2880,7 @@ static void spapr_machine_device_plug(HotplugHandler
> *hotplug_dev,
> error_setg(errp, "Memory hotplug not supported for this
> machine");
> return;
> }
> - node = object_property_get_int(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
> + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
> errp);
> if (*errp) {
> return;
> }
Same.
> diff --git a/numa.c b/numa.c
> index 6fc2393ddd..e32259fedb 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -205,7 +205,7 @@ static void numa_node_parse(NumaNodeOptions *node,
> QemuOpts *opts, Error **errp)
> }
>
> object_ref(o);
> - numa_info[nodenr].node_mem = object_property_get_int(o, "size",
> NULL);
> + numa_info[nodenr].node_mem = object_property_get_uint(o, "size",
> NULL);
> numa_info[nodenr].node_memdev = MEMORY_BACKEND(o);
> }
> numa_info[nodenr].present = true;
> @@ -527,8 +527,8 @@ static int query_memdev(Object *obj, void *opaque)
> m->value->id = object_property_get_str(obj, "id", NULL);
> m->value->has_id = !!m->value->id;
>
> - m->value->size = object_property_get_int(obj, "size",
> - &error_abort);
> + m->value->size = object_property_get_uint(obj, "size",
> + &error_abort);
> m->value->merge = object_property_get_bool(obj, "merge",
> &error_abort);
> m->value->dump = object_property_get_bool(obj, "dump",
I figure "size" is a property of TYPE_MEMORY_BACKEND.
host_memory_backend_get_size() and host_memory_backend_set_size() use
visit_type_size(). Okay.
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5bb8131bb8..eb200ef58b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2324,8 +2324,8 @@ static void x86_cpu_load_def(X86CPU *cpu,
> X86CPUDefinition *def, Error **errp)
> */
>
> /* CPU models only set _minimum_ values for level/xlevel: */
> - object_property_set_int(OBJECT(cpu), def->level, "min-level", errp);
> - object_property_set_int(OBJECT(cpu), def->xlevel, "min-xlevel", errp);
> + object_property_set_uint(OBJECT(cpu), def->level, "min-level", errp);
> + object_property_set_uint(OBJECT(cpu), def->xlevel, "min-xlevel", errp);
>
> object_property_set_int(OBJECT(cpu), def->family, "family", errp);
> object_property_set_int(OBJECT(cpu), def->model, "model", errp);
I figure these are properties of TYPE_X86_CPU, defined with
DEFINE_PROP_UINT32(). Okay.
> diff --git a/ui/console.c b/ui/console.c
> index ac66b3c910..ad3f7c6a2c 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1872,8 +1872,8 @@ QemuConsole *qemu_console_lookup_by_device(DeviceState
> *dev, uint32_t head)
> if (DEVICE(obj) != dev) {
> continue;
> }
> - h = object_property_get_int(OBJECT(consoles[i]),
> - "head", &error_abort);
> + h = object_property_get_uint(OBJECT(consoles[i]),
> + "head", &error_abort);
> if (h != head) {
> continue;
> }
TYPE_QEMU_CONSOLE property "head" is defined with
object_property_add_uint*_ptr(). Okay.
Not your patch's fault, but I need to vent: to read a QOM property, we
call a suitable object_property_get_FOO(), which uses the property's
getter with a new QObject output visitor to get the property value as a
QObject, converts the QObject to the C type for FOO, frees the QObject,
and returns the converted value.
The getter knows the property's C type.
The code reading the property has to know the appropriate FOO for this C
type.
The conversion from QObject to C type does a bit of dynamic type
checking, so the code reading the property screws up too badly, we get
at least a run time failure. There is no protection against messing up
signedness or width, and of course we mess up in places.
Writing is just as convoluted, opaque and error-prone.
This looks like a severe case of OOPitis to me. There must be a better
way! The signedness issues you correct should have been flagged by the
compiler or at least by Coverity. Our OOPitis muddies the waters
sufficiently to defeat both.
- Re: [Qemu-devel] [PATCH 12/17] qdev: use int and uint properties as appropriate, (continued)
[Qemu-devel] [PATCH 15/17] Use uint property getter/setter where appropriate, Marc-André Lureau, 2017/05/09
- Re: [Qemu-devel] [PATCH 15/17] Use uint property getter/setter where appropriate,
Markus Armbruster <=
[Qemu-devel] [PATCH 16/17] RFC: qdict: add uint, Marc-André Lureau, 2017/05/09
[Qemu-devel] [PATCH 17/17] qobject: move dump_qobject() from block/ to qobject/, Marc-André Lureau, 2017/05/09
Re: [Qemu-devel] [PATCH 00/17] qobject/qapi: add uint type, no-reply, 2017/05/13
Re: [Qemu-devel] [PATCH 00/17] qobject/qapi: add uint type, Markus Armbruster, 2017/05/18