Re: [Qemu-trivial] [PATCH v8] qdev: Add support for property type bool

[Juan Quintela <quintela@redhat.com>]

Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> VMState supports the type bool but qdev instead supports bit, backed by
> uint32_t. Therefore let's add PROP_TYPE_BOOL and qdev_prop_set_bool().
>  PropertyInfo qdev_prop_bit = {
> -    .name  = "boolean",
> +    .name  = "bit",

my plan for vmstate was to name this "bool32", as it feels more
descriptive (on QDEV, we can change it, but for qemu for compatibility
reasons, it needs to stay as 4 bytes on the wire, but we can have a bool
on the struct).

This are all the uses of DEFINE_PROP_BIT.

./hw/scsi-disk.c:1736:            DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),

Con be movde to bool

./hw/scsi-disk.c:1780:            DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),

same

./hw/usb-msd.c:658:        DEFINE_PROP_BIT("removable", MSDState, removable, 0, false),

    s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable,
                                            s->conf.bootindex);
*should* be moved to bool

./hw/hpet.c:707:        DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),

it is a bitmap with only one bit defined, not sure about this one.


./hw/virtio.h:213:	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
./hw/virtio.h:215:	DEFINE_PROP_BIT("event_idx", _state, _field, \

This is a real bitmap with 2 values.

./hw/virtio-pci.c:800:            DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
./hw/virtio-pci.c:819:            DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
./hw/virtio-pci.c:843:            DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
./hw/9pfs/virtio-9p-device.c:175:        DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,

Another bitmap with only one defined value.

./hw/virtio-blk.h:103:        DEFINE_PROP_BIT("scsi", _state, _field, VIRTIO_BLK_F_SCSI, true)

It is a bitmap

./hw/ivshmem.c:782:        DEFINE_PROP_BIT("ioeventfd", IVShmemState, features, IVSHMEM_IOEVENTFD, false),
./hw/ivshmem.c:783:        DEFINE_PROP_BIT("msi", IVShmemState,
features, IVSHMEM_MSI, true),

Another bitmap with two values.

./hw/virtio-net.h:172:        DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
./hw/virtio-net.h:173:        DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
./hw/virtio-net.h:174:        DEFINE_PROP_BIT("gso", _state, _field, VIRTIO_NET_F_GSO, true), \
./hw/virtio-net.h:175:        DEFINE_PROP_BIT("guest_tso4", _state, _field, VIRTIO_NET_F_GUEST_TSO4, true), \
./hw/virtio-net.h:176:        DEFINE_PROP_BIT("guest_tso6", _state, _field, VIRTIO_NET_F_GUEST_TSO6, true), \
./hw/virtio-net.h:177:        DEFINE_PROP_BIT("guest_ecn", _state, _field, VIRTIO_NET_F_GUEST_ECN, true), \
./hw/virtio-net.h:178:        DEFINE_PROP_BIT("guest_ufo", _state, _field, VIRTIO_NET_F_GUEST_UFO, true), \
./hw/virtio-net.h:179:        DEFINE_PROP_BIT("host_tso4", _state, _field, VIRTIO_NET_F_HOST_TSO4, true), \
./hw/virtio-net.h:180:        DEFINE_PROP_BIT("host_tso6", _state, _field, VIRTIO_NET_F_HOST_TSO6, true), \
./hw/virtio-net.h:181:        DEFINE_PROP_BIT("host_ecn", _state, _field, VIRTIO_NET_F_HOST_ECN, true), \
./hw/virtio-net.h:182:        DEFINE_PROP_BIT("host_ufo", _state, _field, VIRTIO_NET_F_HOST_UFO, true), \
./hw/virtio-net.h:183:        DEFINE_PROP_BIT("mrg_rxbuf", _state, _field, VIRTIO_NET_F_MRG_RXBUF, true), \
./hw/virtio-net.h:184:        DEFINE_PROP_BIT("status", _state, _field, VIRTIO_NET_F_STATUS, true), \
./hw/virtio-net.h:185:        DEFINE_PROP_BIT("ctrl_vq", _state, _field, VIRTIO_NET_F_CTRL_VQ, true), \
./hw/virtio-net.h:186:        DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
./hw/virtio-net.h:187:        DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
./hw/virtio-net.h:188:        DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true)

Bitmap.

./hw/i8259.c:570:        DEFINE_PROP_BIT("master", PicState, master,  0, false),

BOOL

./hw/pci.c:58:        DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
./hw/pci.c:60:        DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,

bitmap with two values.

./hw/pxa2xx_timer.c:488:        DEFINE_PROP_BIT("tm4", PXA2xxTimerInfo, flags,
./hw/pxa2xx_timer.c:502:        DEFINE_PROP_BIT("tm4", PXA2xxTimerInfo, flags,

Bitmap with only one value.


> +{
> +    bool *ptr = qdev_get_prop_ptr(dev, prop);
> +    if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0) {
> +        *ptr = true;
> +    } else if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0) {
> +        *ptr = false;
> +    } else {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int parse_bool_switch(DeviceState *dev, Property *prop,
> +                             const char *str)
> +{
> +    bool *ptr = qdev_get_prop_ptr(dev, prop);
> +    if (strcmp(str, "on") == 0) {
> +        *ptr = true;
> +    } else if (strcmp(str, "off") == 0) {
> +        *ptr = false;
> +    } else {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}

As I am joining late to this discussion, I am not going to point it very
strong.  But I think that it is an easy to have a single bool type that
accept yes/on/true and no/off/false.  Didn't really see a strong
advantage with the split.

Later, Juan.