[Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().

bool by definition is either true or false. Should the need arise to
parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
a later point in time.

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>
---
 v5 -> v6:
 * Rebased onto QOM properties.
 * Parse and print true/false for bool, leave bit untouched.
 Please review, v6 untested.

 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 |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    4 +++
 2 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 02f0dae..612f2ea 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -96,6 +96,68 @@ 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) {
+        *ptr = true;
+    } else if (strcmp(str, "false") == 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 ? "true" : "false");
+}
+
+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 = "bool",
+    .type = PROP_TYPE_BOOL,
+    .size = sizeof(bool),
+    .parse = parse_bool,
+    .print = print_bool,
+    .get   = get_bool,
+    .set   = set_bool,
+};
+
 /* --- 8bit integer --- */
 
 static int parse_uint8(DeviceState *dev, Property *prop, const char *str)
diff --git a/hw/qdev.h b/hw/qdev.h
index 6b58dd8..5e34d90 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,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;
@@ -315,6 +317,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)                      \
-- 
1.7.7

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Jan Kiszka]

On 2012-01-24 18:17, Andreas Färber 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().
> 
> bool by definition is either true or false. Should the need arise to
> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
> a later point in time.

To make it a real replacement for PROP_TYPE_BIT, let's use on/off, also
for printing. Not only programmers may use this interface. ;)

> 
> 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>
> ---
>  v5 -> v6:
>  * Rebased onto QOM properties.
>  * Parse and print true/false for bool, leave bit untouched.
>  Please review, v6 untested.
> 
>  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 |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h            |    4 +++
>  2 files changed, 66 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 02f0dae..612f2ea 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -96,6 +96,68 @@ 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) {
> +        *ptr = true;
> +    } else if (strcmp(str, "false") == 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 ? "true" : "false");
> +}
> +
> +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 = "bool",

Now we have "bool" and "boolean". Can we rename the latter to bit?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Andreas Färber]

Am 24.01.2012 18:29, schrieb Jan Kiszka:
> On 2012-01-24 18:17, Andreas Färber 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().
>>
>> bool by definition is either true or false. Should the need arise to
>> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
>> a later point in time.
> 
> To make it a real replacement for PROP_TYPE_BIT, let's use on/off, also
> for printing. Not only programmers may use this interface. ;)

Actually non-programmers are the reason for this: cache=on/off makes a
great deal of sense in English (noun), but not enabled=off (adjective),
which was my use case of this patch for ISA devices.

I'm fine with accepting the whole range of possibilities for parsing.
For printing I see no reason to, since there's no legacy users of this
new type we could break. true/false seemed better than yes/no.

>> +PropertyInfo qdev_prop_bool = {
>> +    .name = "bool",
> 
> Now we have "bool" and "boolean". Can we rename the latter to bit?

I'd be fine with that, just don't know if that would break anything
elsewhere for qdev or QOM?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Anthony Liguori]

On 01/24/2012 11:38 AM, Andreas Färber wrote:
> Am 24.01.2012 18:29, schrieb Jan Kiszka:
>> On 2012-01-24 18:17, Andreas Färber 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().
>>>
>>> bool by definition is either true or false. Should the need arise to
>>> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
>>> a later point in time.
>>
>> To make it a real replacement for PROP_TYPE_BIT, let's use on/off, also
>> for printing. Not only programmers may use this interface. ;)
>
> Actually non-programmers are the reason for this: cache=on/off makes a
> great deal of sense in English (noun), but not enabled=off (adjective),
> which was my use case of this patch for ISA devices.
>
> I'm fine with accepting the whole range of possibilities for parsing.
> For printing I see no reason to, since there's no legacy users of this
> new type we could break. true/false seemed better than yes/no.
>
>>> +PropertyInfo qdev_prop_bool = {
>>> +    .name = "bool",
>>
>> Now we have "bool" and "boolean". Can we rename the latter to bit?
>
> I'd be fine with that, just don't know if that would break anything
> elsewhere for qdev or QOM?

Nothing breaks.  Legacy properties (yes, these are legacy) will stick around 
until we stabilize the QOM public interface.

I think that's probably 2.0 material.

Regards,

Anthony Liguori

>
> Andreas
>

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Jan Kiszka]

On 2012-01-24 18:38, Andreas Färber wrote:
> Am 24.01.2012 18:29, schrieb Jan Kiszka:
>> On 2012-01-24 18:17, Andreas Färber 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().
>>>
>>> bool by definition is either true or false. Should the need arise to
>>> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
>>> a later point in time.
>>
>> To make it a real replacement for PROP_TYPE_BIT, let's use on/off, also
>> for printing. Not only programmers may use this interface. ;)
> 
> Actually non-programmers are the reason for this: cache=on/off makes a
> great deal of sense in English (noun), but not enabled=off (adjective),
> which was my use case of this patch for ISA devices.

Agreed, there are use cases for both.

> 
> I'm fine with accepting the whole range of possibilities for parsing.
> For printing I see no reason to, since there's no legacy users of this
> new type we could break. true/false seemed better than yes/no.

We have a few single-bit booleans I would like to convert to bool. So
the best option may be having control over what is printed. Could we
easily provide a PROP_TYPE_SWITCH based on bool?

> 
>>> +PropertyInfo qdev_prop_bool = {
>>> +    .name = "bool",
>>
>> Now we have "bool" and "boolean". Can we rename the latter to bit?
> 
> I'd be fine with that, just don't know if that would break anything
> elsewhere for qdev or QOM?

Hmm, to me it looks like it's only used for verbose error reporting. But
I may miss something.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Andreas Färber]

Am 24.01.2012 18:17, 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().

Actually qdev_prop_set_bool() got lost in a merge conflict... Re-adding.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Stefan Hajnoczi]

On Tue, Jan 24, 2012 at 06:17:36PM +0100, Andreas Färber 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().
> 
> bool by definition is either true or false. Should the need arise to
> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
> a later point in time.
> 
> 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>
> ---
>  v5 -> v6:

Please merge through a qemu.git committer.  v6 and discussion means this
isn't trivial.

Stefan

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Andreas Färber]

Am 27.01.2012 07:23, schrieb Stefan Hajnoczi:
> On Tue, Jan 24, 2012 at 06:17:36PM +0100, Andreas Färber 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().
>>
>> bool by definition is either true or false. Should the need arise to
>> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
>> a later point in time.
>>
>> 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>
>> ---
>>  v5 -> v6:
> 
> Please merge through a qemu.git committer.  v6 and discussion means this
> isn't trivial.

Unfortunately that hasn't happened for half a year though. Generally no
qemu.git committer seems to care about cherry-picking useful
infrastructures from bigger series even once discussions are resolved.

And for the record, it's net v3 as shown in the part you snipped.

Vasilis, unless Jan or someone has further nitpicks, just prepend my v8
to your series and reuse its DEFINE_PROP_BOOL().

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Anthony Liguori]

On 01/27/2012 03:38 AM, Andreas Färber wrote:
> Am 27.01.2012 07:23, schrieb Stefan Hajnoczi:
>> On Tue, Jan 24, 2012 at 06:17:36PM +0100, Andreas Färber 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 DEFINE_PROP_BOOL() and qdev_prop_set_bool().
>>>
>>> bool by definition is either true or false. Should the need arise to
>>> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
>>> a later point in time.
>>>
>>> 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>
>>> ---
>>>   v5 ->  v6:
>>
>> Please merge through a qemu.git committer.  v6 and discussion means this
>> isn't trivial.
>
> Unfortunately that hasn't happened for half a year though. Generally no
> qemu.git committer seems to care about cherry-picking useful
> infrastructures from bigger series even once discussions are resolved.

So... why are we introducing a type that's not being used anywhere?

Are you planning to use this type somewhere or is this purely speculative?

Regards,

Anthony Liguori

>
> And for the record, it's net v3 as shown in the part you snipped.
>
> Vasilis, unless Jan or someone has further nitpicks, just prepend my v8
> to your series and reuse its DEFINE_PROP_BOOL().
>
> Andreas
>

Re: [Qemu-devel] [PATCH v6] qdev: Add support for property type bool

[Andreas Färber]

Am 27.01.2012 13:41, schrieb Anthony Liguori:
> On 01/27/2012 03:38 AM, Andreas Färber wrote:
>> Am 27.01.2012 07:23, schrieb Stefan Hajnoczi:
>>> On Tue, Jan 24, 2012 at 06:17:36PM +0100, Andreas Färber 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 DEFINE_PROP_BOOL() and
>>>> qdev_prop_set_bool().
>>>>
>>>> bool by definition is either true or false. Should the need arise to
>>>> parse yes/no, on/off, 1/0 or whatever as well, we can still add that at
>>>> a later point in time.
>>>>
>>>> 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>
>>>> ---
>>>>   v5 ->  v6:
>>>
>>> Please merge through a qemu.git committer.  v6 and discussion means this
>>> isn't trivial.
>>
>> Unfortunately that hasn't happened for half a year though. Generally no
>> qemu.git committer seems to care about cherry-picking useful
>> infrastructures from bigger series even once discussions are resolved.
> 
> So... why are we introducing a type that's not being used anywhere?
> 
> Are you planning to use this type somewhere or is this purely speculative?

My ISA series and later the 40P series used it (therefore
"cherry-picking" above; additionally it turned out recently that there
had been confusion over who was supposed to pull for PReP - thanks for
recently doing so btw).

Now Jan and I agreed that Vasilis should reuse it for his series in
place of uint32. Therefore I would've appreciated a quick merge (thus
qemu-trivial) so that it can be rebased upon.

Unfortunately I didn't have code for testing all paths handy, my bad.

But nevertheless all these discussions never opposed the core idea, only
the exact way things were printed or parsed. When using
qdev_prop_set_bool() internally, like I did for the pc87312 Super I/O,
none of this matters except for 'info qtree'. So if we could decide on
the DEFINE_PROP_*() macro name, all of the parsing and printing can
still be changed back and forth in a central place including when
someone ventures to translate QEMU to non-English, then everything gets
ugly anyway. :) Correct me if I'm wrong, but as far as I saw, your QOM
series only moved this around and the qdev macros remained.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg