| Message ID | 1327425457-31313-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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 >
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
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
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
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
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 >
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
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) \