Re: [Qemu-devel] [PATCH v2] qdev: Move global validation to a single function

[Don Slutz <dslutz@verizon.com>]

On 06/09/14 14:44, Eduardo Habkost wrote:
> This simplifies the global validation code so all its logic is contained
> in a single function, and fixes the following:
>
>    $ qemu-system-x86_64 -global container.xxx=y
>    hw/core/qdev-properties-system.c:450:qdev_add_one_global: Object 0x7f8d72a03d70 is not an instance of type device
>    Aborted (core dumped)
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

I do not see any more issues with this and it has
passed all my unit testing so:

Tested-by: Don Slutz <dslutz@verizon.com>

    -Don Slutz

> Changes v2:
>   * Rename qdev_prop_check_global() to qdev_prop_check_globals()
>   * Don't generate any warnings from global options that were not
>     user-provided
> ---
>   hw/core/qdev-properties-system.c | 17 +-------------
>   hw/core/qdev-properties.c        | 38 ++++++++++++++++++++++++------
>   include/hw/qdev-core.h           | 10 ++++----
>   include/hw/qdev-properties.h     |  3 ++-
>   tests/test-qdev-global-props.c   | 51 +++++++++++++++++++++++++++++++++++++---
>   vl.c                             |  2 +-
>   6 files changed, 88 insertions(+), 33 deletions(-)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index de433b2..cacd50a 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -439,27 +439,12 @@ PropertyInfo qdev_prop_iothread = {
>   static int qdev_add_one_global(QemuOpts *opts, void *opaque)
>   {
>       GlobalProperty *g;
> -    ObjectClass *oc;
>   
>       g = g_malloc0(sizeof(*g));
>       g->driver   = qemu_opt_get(opts, "driver");
>       g->property = qemu_opt_get(opts, "property");
>       g->value    = qemu_opt_get(opts, "value");
> -    oc = object_class_by_name(g->driver);
> -    if (oc) {
> -        DeviceClass *dc = DEVICE_CLASS(oc);
> -
> -        if (dc->hotpluggable) {
> -            /* If hotpluggable then skip not_used checking. */
> -            g->not_used = false;
> -        } else {
> -            /* Maybe a typo. */
> -            g->not_used = true;
> -        }
> -    } else {
> -        /* Maybe a typo. */
> -        g->not_used = true;
> -    }
> +    g->user_provided = true;
>       qdev_prop_register_global(g);
>       return 0;
>   }
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 3d12560..fc65b7f 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -941,6 +941,14 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
>   static QTAILQ_HEAD(, GlobalProperty) global_props =
>           QTAILQ_HEAD_INITIALIZER(global_props);
>   
> +void qdev_prop_reset_globals(void)
> +{
> +    GlobalProperty *prop, *nextp;
> +    QTAILQ_FOREACH_SAFE(prop, &global_props, next, nextp) {
> +        QTAILQ_REMOVE(&global_props, prop, next);
> +    }
> +}
> +
>   void qdev_prop_register_global(GlobalProperty *prop)
>   {
>       QTAILQ_INSERT_TAIL(&global_props, prop, next);
> @@ -955,19 +963,35 @@ void qdev_prop_register_global_list(GlobalProperty *props)
>       }
>   }
>   
> -int qdev_prop_check_global(void)
> +int qdev_prop_check_globals(void)
>   {
>       GlobalProperty *prop;
>       int ret = 0;
>   
>       QTAILQ_FOREACH(prop, &global_props, next) {
> -        if (!prop->not_used) {
> +        ObjectClass *oc;
> +        DeviceClass *dc;
> +        if (prop->used) {
> +            continue;
> +        }
> +        if (!prop->user_provided) {
> +            continue;
> +        }
> +        oc = object_class_by_name(prop->driver);
> +        oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> +        if (!oc) {
> +            error_report("Warning: global %s.%s has invalid class name",
> +                       prop->driver, prop->property);
> +            ret = 1;
> +            continue;
> +        }
> +        dc = DEVICE_CLASS(oc);
> +        if (!dc->hotpluggable && !prop->used) {
> +            error_report("Warning: global %s.%s=%s\" not used",
> +                       prop->driver, prop->property, prop->value);
> +            ret = 1;
>               continue;
>           }
> -        ret = 1;
> -        error_report("Warning: \"-global %s.%s=%s\" not used",
> -                     prop->driver, prop->property, prop->value);
> -
>       }
>       return ret;
>   }
> @@ -983,7 +1007,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>           if (strcmp(typename, prop->driver) != 0) {
>               continue;
>           }
> -        prop->not_used = false;
> +        prop->used = true;
>           object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
>           if (err != NULL) {
>               error_propagate(errp, err);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 9221cfc..10d7cbc 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -241,16 +241,16 @@ struct PropertyInfo {
>   
>   /**
>    * GlobalProperty:
> - * @not_used: Track use of a global property.  Defaults to false in all C99
> - * struct initializations.
> - *
> - * This prevents reports of .compat_props when they are not used.
> + * @user_provided: Set to true if property comes from user-provided config
> + * (command-line or config file).
> + * @used: Set to true if property was used when initializing a device.
>    */
>   typedef struct GlobalProperty {
>       const char *driver;
>       const char *property;
>       const char *value;
> -    bool not_used;
> +    bool user_provided;
> +    bool used;
>       QTAILQ_ENTRY(GlobalProperty) next;
>   } GlobalProperty;
>   
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index c962b6b..6c30d6d 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -178,9 +178,10 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
>   /* FIXME: Remove opaque pointer properties.  */
>   void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>   
> +void qdev_prop_reset_globals(void);
>   void qdev_prop_register_global(GlobalProperty *prop);
>   void qdev_prop_register_global_list(GlobalProperty *props);
> -int qdev_prop_check_global(void);
> +int qdev_prop_check_globals(void);
>   void qdev_prop_set_globals(DeviceState *dev, Error **errp);
>   void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>                                       Error **errp);
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 2bef04c..8adf284 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -143,18 +143,27 @@ static const TypeInfo dynamic_prop_type = {
>       .class_init = dynamic_class_init,
>   };
>   
> +#define TYPE_NONDEVICE "nondevice-type"
> +
> +static const TypeInfo nondevice_type = {
> +    .name = TYPE_NONDEVICE,
> +    .parent = TYPE_OBJECT,
> +};
> +
>   /* Test setting of static property using global properties */
>   static void test_dynamic_globalprop(void)
>   {
>       MyType *mt;
>       static GlobalProperty props[] = {
> -        { TYPE_DYNAMIC_PROPS, "prop1", "101" },
> -        { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> +        { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
> +        { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
>           { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
> +        { TYPE_NONDEVICE, "prop4", "104", true },
>           {}
>       };
>       int all_used;
>   
> +    qdev_prop_reset_globals();
>       qdev_prop_register_global_list(props);
>   
>       mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
> @@ -162,8 +171,41 @@ static void test_dynamic_globalprop(void)
>   
>       g_assert_cmpuint(mt->prop1, ==, 101);
>       g_assert_cmpuint(mt->prop2, ==, 102);
> -    all_used = qdev_prop_check_global();
> +    all_used = qdev_prop_check_globals();
>       g_assert_cmpuint(all_used, ==, 1);
> +    g_assert(props[0].used);
> +    g_assert(props[1].used);
> +    g_assert(!props[2].used);
> +    g_assert(!props[3].used);
> +}
> +
> +/* Test setting of static property using global properties, not user-provided */
> +static void test_dynamic_globalprop_nouser(void)
> +{
> +    MyType *mt;
> +    static GlobalProperty props[] = {
> +        { TYPE_DYNAMIC_PROPS, "prop1", "101" },
> +        { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> +        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" },
> +        { TYPE_NONDEVICE, "prop4", "104" },
> +        {}
> +    };
> +    int all_used;
> +
> +    qdev_prop_reset_globals();
> +    qdev_prop_register_global_list(props);
> +
> +    mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
> +    qdev_init_nofail(DEVICE(mt));
> +
> +    g_assert_cmpuint(mt->prop1, ==, 101);
> +    g_assert_cmpuint(mt->prop2, ==, 102);
> +    all_used = qdev_prop_check_globals();
> +    g_assert_cmpuint(all_used, ==, 0);
> +    g_assert(props[0].used);
> +    g_assert(props[1].used);
> +    g_assert(!props[2].used);
> +    g_assert(!props[3].used);
>   }
>   
>   int main(int argc, char **argv)
> @@ -173,10 +215,13 @@ int main(int argc, char **argv)
>       module_call_init(MODULE_INIT_QOM);
>       type_register_static(&static_prop_type);
>       type_register_static(&dynamic_prop_type);
> +    type_register_static(&nondevice_type);
>   
>       g_test_add_func("/qdev/properties/static/default", test_static_prop);
>       g_test_add_func("/qdev/properties/static/global", test_static_globalprop);
>       g_test_add_func("/qdev/properties/dynamic/global", test_dynamic_globalprop);
> +    g_test_add_func("/qdev/properties/dynamic/global/nouser",
> +                    test_dynamic_globalprop_nouser);
>   
>       g_test_run();
>   
> diff --git a/vl.c b/vl.c
> index a3def97..7eb1d1c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4541,7 +4541,7 @@ int main(int argc, char **argv, char **envp)
>           }
>       }
>   
> -    qdev_prop_check_global();
> +    qdev_prop_check_globals();
>   
>       if (incoming) {
>           Error *local_err = NULL;