diff mbox

[v4,01/12] qdev: Add support for property type bool

Message ID 1307559319-16183-2-git-send-email-andreas.faerber@web.de
State New
Headers show

Commit Message

Andreas Färber June 8, 2011, 6:55 p.m. UTC 
VMState supports the type bool but qdev instead supports bit, backed by
uint32_t. Therefore let's add DEFINE_PROP_BOOL() and qdev_prop_set_bool().

Since, e.g., enabled=on does not look nice, parse/print "yes" and "no".

Cc: Juan Quintela <quintela@redhat.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/qdev-properties.c |   35 +++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    5 +++++
 2 files changed, 40 insertions(+), 0 deletions(-)

Comments

Markus Armbruster June 9, 2011, 2:45 p.m. UTC | #1 
Andreas Färber <andreas.faerber@web.de> writes:

> VMState supports the type bool but qdev instead supports bit, backed by
> uint32_t. Therefore let's add DEFINE_PROP_BOOL() and qdev_prop_set_bool().
>
> Since, e.g., enabled=on does not look nice, parse/print "yes" and "no".

The difference between bool and bit properties is implementation detail.
Using "on/off" for one, and "yes/no" for the other is a gratuitously
inconsistent user interface.

For what it's worth, "on" doesn't look nice for many bit properties
either.

Options:

1. Live with the inconsistent user interface

1. Stick to on/off ugly or not

3. Switch to yes/no and deprecate on/off

Opinions?
Andreas Färber June 12, 2011, 11:44 a.m. UTC | #2 
Am 09.06.2011 um 16:45 schrieb Markus Armbruster:

> Andreas Färber <andreas.faerber@web.de> writes:
>
>> VMState supports the type bool but qdev instead supports bit,  
>> backed by
>> uint32_t. Therefore let's add DEFINE_PROP_BOOL() and  
>> qdev_prop_set_bool().
>>
>> Since, e.g., enabled=on does not look nice, parse/print "yes" and  
>> "no".
>
> The difference between bool and bit properties is implementation  
> detail.
> Using "on/off" for one, and "yes/no" for the other is a gratuitously
> inconsistent user interface.
>
> For what it's worth, "on" doesn't look nice for many bit properties
> either.
>
> Options:
>
> 1. Live with the inconsistent user interface
>
> 1. Stick to on/off ugly or not
>
> 3. Switch to yes/no and deprecate on/off
>
> Opinions?

It would certainly be possible to support all of on/off, yes/no, true/ 
false, 1/0 in the two parsing methods. Independent of any  
implementation detail.

My reasoning was that grammatically, using on/off with a noun in just  
fine, e.g., cache=off.
With an adjective however, it's wrong. Therefore my choice of yes/no  
here.

Juan, what did you think of?

Andreas
diff mbox

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index eff2d24..e262f9a 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -63,6 +63,36 @@  PropertyInfo qdev_prop_bit = {
     .print = print_bit,
 };
 
+/* --- bool --- */
+
+static int parse_bool(DeviceState *dev, Property *prop, const char *str)
+{
+    bool *ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (strncasecmp(str, "yes", 3) == 0) {
+        *ptr = true;
+    } else if (strncasecmp(str, "no", 2) == 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");
+}
+
+PropertyInfo qdev_prop_bool = {
+    .name = "yes/no",
+    .type = PROP_TYPE_BOOL,
+    .size = sizeof(bool),
+    .parse = parse_bool,
+    .print = print_bool,
+};
+
 /* --- 8bit integer --- */
 
 static int parse_uint8(DeviceState *dev, Property *prop, const char *str)
@@ -644,6 +674,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 8a13ec9..f05166d 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -101,6 +101,7 @@  enum PropertyType {
     PROP_TYPE_VLAN,
     PROP_TYPE_PTR,
     PROP_TYPE_BIT,
+    PROP_TYPE_BOOL,
 };
 
 struct PropertyInfo {
@@ -219,6 +220,7 @@  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_uint8;
 extern PropertyInfo qdev_prop_uint16;
 extern PropertyInfo qdev_prop_uint32;
@@ -257,6 +259,8 @@  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_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)                      \
@@ -298,6 +302,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);