Re: [Qemu-devel] [PATCH 10/12] qdev: remove hex8/32/64 property types

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7380 bytes --]

On 01/30/2014 06:09 AM, Paolo Bonzini wrote:
> Replace them with uint8/32/64.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/hw/audio/pcspk.c
> @@ -181,7 +181,7 @@ static void pcspk_realizefn(DeviceState *dev, Error **errp)
>  }
>  
>  static Property pcspk_properties[] = {
> -    DEFINE_PROP_HEX32("iobase", PCSpkState, iobase,  -1),
> +    DEFINE_PROP_UINT32("iobase", PCSpkState, iobase,  -1),

When there's multiple properties, I can see the value of alignment.  But
for this instance, the two spaces before -1 seem spurious; you could fix
them in this patch.


> +++ b/hw/char/parallel.c
> @@ -595,7 +595,7 @@ bool parallel_mm_init(MemoryRegion *address_space,
>  
>  static Property parallel_isa_properties[] = {
>      DEFINE_PROP_UINT32("index", ISAParallelState, index,   -1),
> -    DEFINE_PROP_HEX32("iobase", ISAParallelState, iobase,  -1),
> +    DEFINE_PROP_UINT32("iobase", ISAParallelState, iobase,  -1),
>      DEFINE_PROP_UINT32("irq",   ISAParallelState, isairq,  7),
>      DEFINE_PROP_CHR("chardev",  ISAParallelState, state.chr),

And alignment looks all messed up on this one.

>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
> index 5cb77b3..c9fcb27 100644
> --- a/hw/char/serial-isa.c
> +++ b/hw/char/serial-isa.c
> @@ -88,7 +88,7 @@ static const VMStateDescription vmstate_isa_serial = {
>  
>  static Property serial_isa_properties[] = {
>      DEFINE_PROP_UINT32("index",  ISASerialState, index,   -1),
> -    DEFINE_PROP_HEX32("iobase",  ISASerialState, iobase,  -1),
> +    DEFINE_PROP_UINT32("iobase",  ISASerialState, iobase,  -1),

Drop a space before ISASerialState to make this one look nice.


> +++ b/hw/display/tcx.c
> @@ -617,11 +617,11 @@ static int tcx_init1(SysBusDevice *dev)
>  }
>  
>  static Property tcx_properties[] = {
> -    DEFINE_PROP_HEX32("vram_size", TCXState, vram_size, -1),
> +    DEFINE_PROP_UINT32("vram_size", TCXState, vram_size, -1),
>      DEFINE_PROP_UINT16("width",    TCXState, width,     -1),
>      DEFINE_PROP_UINT16("height",   TCXState, height,    -1),
>      DEFINE_PROP_UINT16("depth",    TCXState, depth,     -1),
> -    DEFINE_PROP_HEX64("prom_addr", TCXState, prom_addr, -1),
> +    DEFINE_PROP_UINT64("prom_addr", TCXState, prom_addr, -1),

Another one where alignment is now funky.


> +++ b/hw/ide/isa.c
> @@ -104,8 +104,8 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>  }
>  
>  static Property isa_ide_properties[] = {
> -    DEFINE_PROP_HEX32("iobase",  ISAIDEState, iobase,  0x1f0),
> -    DEFINE_PROP_HEX32("iobase2", ISAIDEState, iobase2, 0x3f6),
> +    DEFINE_PROP_UINT32("iobase",  ISAIDEState, iobase,  0x1f0),
> +    DEFINE_PROP_UINT32("iobase2", ISAIDEState, iobase2, 0x3f6),
>      DEFINE_PROP_UINT32("irq",    ISAIDEState, isairq,  14),

And another one.

>      DEFINE_PROP_END_OF_LIST(),
>  };
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 18c4b7e..6e475e6 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -206,7 +206,7 @@ static int ide_drive_initfn(IDEDevice *dev)
>  #define DEFINE_IDE_DEV_PROPERTIES()                     \
>      DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
>      DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
> -    DEFINE_PROP_HEX64("wwn",  IDEDrive, dev.wwn, 0),    \
> +    DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),    \

Here, the trailing \ lost alignment.

> +++ b/hw/intc/i8259_common.c
> @@ -123,9 +123,9 @@ static const VMStateDescription vmstate_pic_common = {
>  };
>  
>  static Property pic_properties_common[] = {
> -    DEFINE_PROP_HEX32("iobase", PICCommonState, iobase,  -1),
> -    DEFINE_PROP_HEX32("elcr_addr", PICCommonState, elcr_addr,  -1),
> -    DEFINE_PROP_HEX8("elcr_mask", PICCommonState, elcr_mask,  -1),
> +    DEFINE_PROP_UINT32("iobase", PICCommonState, iobase,  -1),
> +    DEFINE_PROP_UINT32("elcr_addr", PICCommonState, elcr_addr,  -1),
> +    DEFINE_PROP_UINT8("elcr_mask", PICCommonState, elcr_mask,  -1),
>      DEFINE_PROP_BIT("master", PICCommonState, master,  0, false),

Here there's no alignment, but lots of doubled spaces.

> +++ b/hw/misc/applesmc.c
> @@ -250,7 +250,7 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp)
>  }
>  
>  static Property applesmc_isa_properties[] = {
> -    DEFINE_PROP_HEX32("iobase", AppleSMCState, iobase,
> +    DEFINE_PROP_UINT32("iobase", AppleSMCState, iobase,
>                        APPLESMC_DEFAULT_IOBASE),

Indentation is now off.

> +++ b/hw/net/ne2000-isa.c
> @@ -86,7 +86,7 @@ static void isa_ne2000_realizefn(DeviceState *dev, Error **errp)
>  }
>  
>  static Property ne2000_isa_properties[] = {
> -    DEFINE_PROP_HEX32("iobase", ISANE2000State, iobase, 0x300),
> +    DEFINE_PROP_UINT32("iobase", ISANE2000State, iobase, 0x300),
>      DEFINE_PROP_UINT32("irq",   ISANE2000State, isairq, 9),
>      DEFINE_NIC_PROPERTIES(ISANE2000State, ne2000.c),

Here, the line you didn't touch looks awkward.

> +++ b/hw/sd/sdhci.c
> @@ -1234,9 +1234,9 @@ const VMStateDescription sdhci_vmstate = {
>  /* Capabilities registers provide information on supported features of this
>   * specific host controller implementation */
>  static Property sdhci_properties[] = {
> -    DEFINE_PROP_HEX32("capareg", SDHCIState, capareg,
> +    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>              SDHC_CAPAB_REG_DEFAULT),

Might as well fix indentation while touching this.


> +++ b/hw/timer/i8254.c
> @@ -342,7 +342,7 @@ static void pit_realizefn(DeviceState *dev, Error **err)
>  }
>  
>  static Property pit_properties[] = {
> -    DEFINE_PROP_HEX32("iobase", PITCommonState, iobase,  -1),
> +    DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
>      DEFINE_PROP_END_OF_LIST(),

Another instance of unneeded double space.


> +++ b/hw/usb/host-libusb.c
> @@ -1324,8 +1324,8 @@ static Property usb_host_dev_properties[] = {
>      DEFINE_PROP_UINT32("hostbus",  USBHostDevice, match.bus_num,    0),
>      DEFINE_PROP_UINT32("hostaddr", USBHostDevice, match.addr,       0),
>      DEFINE_PROP_STRING("hostport", USBHostDevice, match.port),
> -    DEFINE_PROP_HEX32("vendorid",  USBHostDevice, match.vendor_id,  0),
> -    DEFINE_PROP_HEX32("productid", USBHostDevice, match.product_id, 0),
> +    DEFINE_PROP_UINT32("vendorid",  USBHostDevice, match.vendor_id,  0),
> +    DEFINE_PROP_UINT32("productid", USBHostDevice, match.product_id, 0),
>      DEFINE_PROP_UINT32("isobufs",  USBHostDevice, iso_urb_count,    4),

Alignment looks off.


> +++ b/include/hw/qdev-dma.h
> @@ -7,4 +7,4 @@
>   * See the COPYING file in the top-level directory.
>   */
>  #define DEFINE_PROP_DMAADDR(_n, _s, _f, _d)                               \
> -    DEFINE_PROP_HEX64(_n, _s, _f, _d)
> +    DEFINE_PROP_UINT64(_n, _s, _f, _d)

Do we even need this #define, or would it be smarter to just inline the
use of DEFINE_PROP_UINT64 in the rest of the file?

All my comments are cosmetic, so my review still stands; I'm fine
whether you apply this as-is or touch it up per the suggestions.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]