[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/17] qdev: remove PropertyInfo.qtype field
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 01/17] qdev: remove PropertyInfo.qtype field |
Date: |
Tue, 9 May 2017 13:40:49 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
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 <address@hidden>
> ---
> 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
signature.asc
Description: OpenPGP digital signature