Re: [Qemu-devel] [PATCH 01/17] qdev: remove PropertyInfo.qtype field

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

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.lureau@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(-)
> 

> +++ 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);

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().

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]