Cc Paolo, because I have a question for him at the end.
Eric Blake <ebl...@redhat.com> writes:
> On 05/09/2017 12:35 PM, Marc-André Lureau wrote:
>> Remove dependency on qapi qtype, replace a field by a few helper
>> functions to determine the default value type (introduced in commit
>> 4f2d3d7).
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> ---
>> include/hw/qdev-core.h | 1 -
>> include/hw/qdev-properties.h | 5 -----
>> hw/core/qdev.c | 32 ++++++++++++++++++++++++++------
>> 3 files changed, 26 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 4bf86b0ad8..0f21a500cd 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -225,7 +225,6 @@ struct Property {
>> PropertyInfo *info;
>> ptrdiff_t offset;
>> uint8_t bitnr;
>> - QType qtype;
>> int64_t defval;
>> int arrayoffset;
>> PropertyInfo *arrayinfo;
As we'll see in the rest of the patch, member qtype is only used by
qdev_property_add_static(). It has nothing to do with QObject there, it
merely helps sorting properties into buckets "uninteresting", "boolean",
"enumeration", "integer".
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 1d69fa7a8f..16d5d0629b 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -42,7 +42,6 @@ extern PropertyInfo qdev_prop_arraylen;
#define DEFINE_PROP_DEFAULT(_name, _state, _field, _defval, _prop, _type) {
\
.name = (_name), \
>> .info = &(_prop), \
>> .offset = offsetof(_state, _field) \
>> + type_check(_type,typeof_field(_state, _field)), \
>> - .qtype = QTYPE_QINT, \
>> .defval = (_type)_defval, \
>> }
>> #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { \
>> @@ -51,7 +50,6 @@ extern PropertyInfo qdev_prop_arraylen;
#define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { \
.name = (_name), \
.info = &(qdev_prop_bit), \
>> .bitnr = (_bit), \
>> .offset = offsetof(_state, _field) \
>> + type_check(uint32_t,typeof_field(_state, _field)), \
>> - .qtype = QTYPE_QBOOL, \
>> .defval = (bool)_defval, \
>> }
>> #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) { \
>> @@ -60,7 +58,6 @@ extern PropertyInfo qdev_prop_arraylen;
#define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) { \
.name = (_name), \
.info = &(qdev_prop_bit64), \
>> .bitnr = (_bit), \
>> .offset = offsetof(_state, _field) \
>> + type_check(uint64_t, typeof_field(_state, _field)), \
>> - .qtype = QTYPE_QBOOL, \
>> .defval = (bool)_defval, \
>> }
>>
>> @@ -69,7 +66,6 @@ extern PropertyInfo qdev_prop_arraylen;
#define DEFINE_PROP_BOOL(_name, _state, _field, _defval) { \
.name = (_name), \
>> .info = &(qdev_prop_bool), \
>> .offset = offsetof(_state, _field) \
>> + type_check(bool, typeof_field(_state, _field)), \
>> - .qtype = QTYPE_QBOOL, \
>> .defval = (bool)_defval, \
>> }
>>
>> @@ -105,7 +101,6 @@ extern PropertyInfo qdev_prop_arraylen;>
#define DEFINE_PROP_ARRAY(_name, _state, _field, \
_arrayfield, _arrayprop, _arraytype) { \
.name = (PROP_ARRAY_LEN_PREFIX _name), \
.info = &(qdev_prop_arraylen), \
>> .offset = offsetof(_state, _field) \
>> + type_check(uint32_t, typeof_field(_state, _field)), \
>> - .qtype = QTYPE_QINT, \
>> .arrayinfo = &(_arrayprop), \
>> .arrayfieldsize = sizeof(_arraytype), \
>> .arrayoffset = offsetof(_state, _arrayfield), \
Note for later:
* Member qtype can be QTYPE_QINT, QTYPE_QBOOL or zero (no initializer),
i.e. QTYPE_NONE.
* Properties defined with DEFINE_PROP_BIT(), DEFINE_PROP_BIT64() and
DEFINE_PROP_BOOL() are QTYPE_QBOOL. These are all boolean properties.
I didn't check whether the converse is also true, i.e. all boolean
properties are defined that way.
* Properties defined with DEFINE_PROP_DEFAULT() and DEFINE_PROP_ARRAY()
are QTYPE_QINT. On closer examination, the former are all integer and
enumeration properties, and the latter are all arrays of integer. I
didn't check whether the converse is also true.
>> +++ b/hw/core/qdev.c
>> @@ -755,6 +755,30 @@ static void qdev_property_add_legacy(DeviceState *dev,
>> Property *prop,
>> g_free(name);
>> }
>>
>> +static bool prop_info_is_bool(const PropertyInfo *info)
>> +{
>> + return info == &qdev_prop_bit
>> + || info == &qdev_prop_bit64
>> + || info == &qdev_prop_bool;
>> +}
>
> I guess we can expand these helpers if we add more property types later.
>
>> +
>> +static bool prop_info_is_int(const PropertyInfo *info)
>> +{
>> + return info == &qdev_prop_uint8
>> + || info == &qdev_prop_uint16
>> + || info == &qdev_prop_uint32
>> + || info == &qdev_prop_int32
>> + || info == &qdev_prop_uint64
>
> Interesting dissimilarity between existing types, but not the fault of
> your patch.
>
>> + || info == &qdev_prop_size
>> + || info == &qdev_prop_pci_devfn
>
> Okay so far.
>
>> + || info == &qdev_prop_on_off_auto
>> + || info == &qdev_prop_losttickpolicy
>> + || info == &qdev_prop_blockdev_on_error
>> + || info == &qdev_prop_bios_chs_trans
>
> Aren't these four enums rather than ints?
>
>> + || info == &qdev_prop_blocksize
>> + || info == &qdev_prop_arraylen;
>> +}
>> +
>
>> @@ -794,16 +818,12 @@ void qdev_property_add_static(DeviceState *dev,
>> Property *prop,
>> prop->info->description,
>> &error_abort);
>>
>> - if (prop->qtype == QTYPE_NONE) {
>> - return;
>> - }
>> -
>> - if (prop->qtype == QTYPE_QBOOL) {
>> + if (prop_info_is_bool(prop->info)) {
>> object_property_set_bool(obj, prop->defval, prop->name,
>> &error_abort);
>> } else if (prop->info->enum_table) {
>> object_property_set_str(obj, prop->info->enum_table[prop->defval],
>> prop->name, &error_abort);
>> - } else if (prop->qtype == QTYPE_QINT) {
>> + } else if (prop_info_is_int(prop->info)) {
>> object_property_set_int(obj, prop->defval, prop->name,
>> &error_abort);
Old code:
* Property is either uninteresting (QTYPE_NONE), boolean (QTYPE_QBOOL),
integer, enumeration or array of integer (QTYPE_QINT)
* If uninteresting (QTYPE_NONE), do nothing.
* If boolean (QTYPE_QBOOL), object_property_set_bool()
* If enumeration (QTYPE_QINT and info->enum_table),
object_property_set_str()
* If integer or array of integer (QTYPE_QINT and not info->enum_table),
object_property_set_int()
The patch drops the check of QTYPE_NONE. No change as long as
info->enum_table implies QTYPE_QINT. Plausible, but we need to
double-check.
> So enum_table has priority over prop_info_is_int(), in which case, the
> four types I pointed out as being enums will still use set_str() rather
> than set_int().
Yes.
I'm not sure I fully understand this code's logic. It's from Paolo's
commit 4f2d3d7, moved here in commit fdae245.
> I'm not necessarily sold on this patch - previously, the type was local
> to the declaration of the struct (you could tell by reading
> qdev_prop_bit that it was a boolean type); now the type is remote (you
> have to hunt elsewhere to see how the property is categorized). I'm not
> rejecting it (I see how getting rid of a dependency on QType makes it
> easier for the rest of the series to change QType underpinnings), but
> wonder if that is a strong enough justification.
>
> But if we DO keep it, you'll want a v2 that cleans up the
> prop_info_is_int() impedance mismatch.
The patch cleans up a mild abuse of QType for another purpose. I like
that.
What I don't like is enumerating PropertyInfo in helpers. A relatively
straightforward way to avoid this would be moving the part of
qdev_property_add_static() that varies between properties into a new
PropertyInfo method. Assumes that *all* instances of the same
PropertyInfo should behave the same. Paolo, is that the case?
