| Message ID | 1327593288-22521-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
Am 26.01.2012 16:54, schrieb Andreas Färber: > 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(). > > With non-programmers in mind, instead of universal true/false provide > two different property types: > * on/off for DEFINE_PROP_SWITCH() (requested by Jan) and > * yes/no for DEFINE_PROP_BOOL() (also accepting true/false as input). > > Signed-off-by: Andreas Färber <andreas.faerber@web.de> > Cc: Juan Quintela <quintela@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Jan Kiszka <jan.kiszka@siemens.com> > Cc: Vasilis Liaskovitis <vasilis.liaskovitis@profitbricks.com> > --- > v7 -> v8: > * Add missing extern qdev_prop_bool_switch for DEFINE_PROP_SWITCH(). > > v6 -> v7: > * Rename existing "boolean" type to "bit". > * Re-introduce qdev_prop_set_bool(). > * Split "bool" into "switch" (on/off) and "boolean" (yes/no; true/false input). > > v5 -> v6: > * Rebased onto QOM properties. > * Parse and print true/false for bool, leave bit untouched. > Please review, v6 untested. > [* Accidentally dropped qdev_prop_set_bool().] > > v4 -> v5 (40P): > * Parse on/off in addition to yes/no for both bit and bool, print yes/no for bool. > > v4 (ISA): > * Introduced. > hw/qdev-properties.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++- > hw/qdev.h | 8 ++++ > 2 files changed, 109 insertions(+), 1 deletions(-) > > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > index 02f0dae..0b1e9ea 100644 > --- a/hw/qdev-properties.c > +++ b/hw/qdev-properties.c > @@ -86,7 +86,7 @@ static void set_bit(DeviceState *dev, Visitor *v, void *opaque, > } > > PropertyInfo qdev_prop_bit = { > - .name = "boolean", > + .name = "bit", > .legacy_name = "on/off", > .type = PROP_TYPE_BIT, > .size = sizeof(uint32_t), > @@ -96,6 +96,101 @@ PropertyInfo qdev_prop_bit = { > .set = set_bit, > }; > > +/* --- bool --- */ > + > +static int parse_bool(DeviceState *dev, Property *prop, const char *str) > +{ > + 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) { I just noticed that Jan's patch does strncasecmp() for bit, so if we want those semantics here too (foo=TRUE,bar=False,foobar=yES) I can send a v9. I really hate replying to myself though, so I'll wait a bit for other review comments to avoid a further surge in patch revisions. Andreas > + *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; > +} > + > +static int print_bool(DeviceState *dev, Property *prop, > + char *dest, size_t len) > +{ > + bool *ptr = qdev_get_prop_ptr(dev, prop); > + return snprintf(dest, len, *ptr ? "yes" : "no"); > +} > + > +static int print_bool_switch(DeviceState *dev, Property *prop, > + char *dest, size_t len) > +{ > + bool *ptr = qdev_get_prop_ptr(dev, prop); > + return snprintf(dest, len, *ptr ? "on" : "off"); > +} > + > +static void get_bool(DeviceState *dev, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + Property *prop = opaque; > + bool *ptr = qdev_get_prop_ptr(dev, prop); > + > + visit_type_bool(v, ptr, name, errp); > +} > + > +static void set_bool(DeviceState *dev, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + Property *prop = opaque; > + bool *ptr = qdev_get_prop_ptr(dev, prop); > + Error *local_err = NULL; > + bool value; > + > + if (dev->state != DEV_STATE_CREATED) { > + error_set(errp, QERR_PERMISSION_DENIED); > + return; > + } > + > + visit_type_bool(v, &value, name, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + *ptr = value; > +} > + > +PropertyInfo qdev_prop_bool = { > + .name = "boolean", > + .type = PROP_TYPE_BOOL, > + .size = sizeof(bool), > + .parse = parse_bool, > + .print = print_bool, > + .get = get_bool, > + .set = set_bool, > +}; > + > +PropertyInfo qdev_prop_bool_switch = { > + .name = "switch", > + .type = PROP_TYPE_BOOL, > + .size = sizeof(bool), > + .parse = parse_bool_switch, > + .print = print_bool_switch, > + .get = get_bool, > + .set = set_bool, > +}; > + > /* --- 8bit integer --- */ > > static int parse_uint8(DeviceState *dev, Property *prop, const char *str) > @@ -1055,6 +1150,11 @@ void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value) > qdev_prop_set(dev, name, &value, PROP_TYPE_BIT); > } > > +void qdev_prop_set_bool(DeviceState *dev, const char *name, bool value) > +{ > + qdev_prop_set(dev, name, &value, PROP_TYPE_BOOL); > +} > + > void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value) > { > qdev_prop_set(dev, name, &value, PROP_TYPE_UINT8); > diff --git a/hw/qdev.h b/hw/qdev.h > index 6b58dd8..cef20f6 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -152,6 +152,7 @@ enum PropertyType { > PROP_TYPE_VLAN, > PROP_TYPE_PTR, > PROP_TYPE_BIT, > + PROP_TYPE_BOOL, > }; > > struct PropertyInfo { > @@ -276,6 +277,8 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data); > /*** qdev-properties.c ***/ > > extern PropertyInfo qdev_prop_bit; > +extern PropertyInfo qdev_prop_bool; > +extern PropertyInfo qdev_prop_bool_switch; > extern PropertyInfo qdev_prop_uint8; > extern PropertyInfo qdev_prop_uint16; > extern PropertyInfo qdev_prop_uint32; > @@ -315,6 +318,10 @@ extern PropertyInfo qdev_prop_pci_devfn; > .defval = (bool[]) { (_defval) }, \ > } > > +#define DEFINE_PROP_BOOL(_n, _s, _f, _d) \ > + DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_bool, bool) > +#define DEFINE_PROP_SWITCH(_n, _s, _f, _d) \ > + DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_bool_switch, bool) > #define DEFINE_PROP_UINT8(_n, _s, _f, _d) \ > DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t) > #define DEFINE_PROP_UINT16(_n, _s, _f, _d) \ > @@ -358,6 +365,7 @@ int qdev_prop_exists(DeviceState *dev, const char *name); > int qdev_prop_parse(DeviceState *dev, const char *name, const char *value); > void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type); > void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value); > +void qdev_prop_set_bool(DeviceState *dev, const char *name, bool value); > void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value); > void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value); > void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value);
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.
On 2012-01-27 13:17, Juan Quintela wrote: >> +{ >> + 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. Accepting all this on input is a non-issue, but true/false as output is suboptimal for quite a few existing and future switches. Jan
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 02f0dae..0b1e9ea 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -86,7 +86,7 @@ static void set_bit(DeviceState *dev, Visitor *v, void *opaque, } PropertyInfo qdev_prop_bit = { - .name = "boolean", + .name = "bit", .legacy_name = "on/off", .type = PROP_TYPE_BIT, .size = sizeof(uint32_t), @@ -96,6 +96,101 @@ PropertyInfo qdev_prop_bit = { .set = set_bit, }; +/* --- bool --- */ + +static int parse_bool(DeviceState *dev, Property *prop, const char *str) +{ + 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; +} + +static int print_bool(DeviceState *dev, Property *prop, + char *dest, size_t len) +{ + bool *ptr = qdev_get_prop_ptr(dev, prop); + return snprintf(dest, len, *ptr ? "yes" : "no"); +} + +static int print_bool_switch(DeviceState *dev, Property *prop, + char *dest, size_t len) +{ + bool *ptr = qdev_get_prop_ptr(dev, prop); + return snprintf(dest, len, *ptr ? "on" : "off"); +} + +static void get_bool(DeviceState *dev, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + Property *prop = opaque; + bool *ptr = qdev_get_prop_ptr(dev, prop); + + visit_type_bool(v, ptr, name, errp); +} + +static void set_bool(DeviceState *dev, Visitor *v, void *opaque, + const char *name, Error **errp) +{ + Property *prop = opaque; + bool *ptr = qdev_get_prop_ptr(dev, prop); + Error *local_err = NULL; + bool value; + + if (dev->state != DEV_STATE_CREATED) { + error_set(errp, QERR_PERMISSION_DENIED); + return; + } + + visit_type_bool(v, &value, name, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + *ptr = value; +} + +PropertyInfo qdev_prop_bool = { + .name = "boolean", + .type = PROP_TYPE_BOOL, + .size = sizeof(bool), + .parse = parse_bool, + .print = print_bool, + .get = get_bool, + .set = set_bool, +}; + +PropertyInfo qdev_prop_bool_switch = { + .name = "switch", + .type = PROP_TYPE_BOOL, + .size = sizeof(bool), + .parse = parse_bool_switch, + .print = print_bool_switch, + .get = get_bool, + .set = set_bool, +}; + /* --- 8bit integer --- */ static int parse_uint8(DeviceState *dev, Property *prop, const char *str) @@ -1055,6 +1150,11 @@ void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value) qdev_prop_set(dev, name, &value, PROP_TYPE_BIT); } +void qdev_prop_set_bool(DeviceState *dev, const char *name, bool value) +{ + qdev_prop_set(dev, name, &value, PROP_TYPE_BOOL); +} + void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value) { qdev_prop_set(dev, name, &value, PROP_TYPE_UINT8); diff --git a/hw/qdev.h b/hw/qdev.h index 6b58dd8..cef20f6 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -152,6 +152,7 @@ enum PropertyType { PROP_TYPE_VLAN, PROP_TYPE_PTR, PROP_TYPE_BIT, + PROP_TYPE_BOOL, }; struct PropertyInfo { @@ -276,6 +277,8 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data); /*** qdev-properties.c ***/ extern PropertyInfo qdev_prop_bit; +extern PropertyInfo qdev_prop_bool; +extern PropertyInfo qdev_prop_bool_switch; extern PropertyInfo qdev_prop_uint8; extern PropertyInfo qdev_prop_uint16; extern PropertyInfo qdev_prop_uint32; @@ -315,6 +318,10 @@ extern PropertyInfo qdev_prop_pci_devfn; .defval = (bool[]) { (_defval) }, \ } +#define DEFINE_PROP_BOOL(_n, _s, _f, _d) \ + DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_bool, bool) +#define DEFINE_PROP_SWITCH(_n, _s, _f, _d) \ + DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_bool_switch, bool) #define DEFINE_PROP_UINT8(_n, _s, _f, _d) \ DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t) #define DEFINE_PROP_UINT16(_n, _s, _f, _d) \ @@ -358,6 +365,7 @@ int qdev_prop_exists(DeviceState *dev, const char *name); int qdev_prop_parse(DeviceState *dev, const char *name, const char *value); void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type); void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value); +void qdev_prop_set_bool(DeviceState *dev, const char *name, bool value); void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value); void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value); void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value);