Re: [Qemu-devel] [PATCH v2 07/10] qdev: Eliminate "global not used" warning

[Igor Mammedov]

On Mon, 20 Jun 2016 12:53:00 -0300
Eduardo Habkost <address@hidden> wrote:

> qdev_prop_check_globals() tries to warn the user if a given
> -global option was not used. But it does that only if the device
> is not hotpluggable.
> 
> The warning also makes it harder for management code or people
> that write their own scripts or config files: there's no way to
> know if a given -global option will trigger a warning or not,
> because we don't have any introspection mechanism to check if a
> machine-type instantiates a given device or not.
> 
> The warning is also the only reason we have the 'user_provided'
> and 'used' fields in struct GlobalProperty. Removing the check
> will let us simplify the code.
> 
> In other words, the warning is nearly useless and gets in our way
> and our users' way. Remove it.
> 
> With this change, we don't need subprocess tricks in
> test-qdev-global-props anymore.
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
Reviewed-by: Igor Mammedov <address@hidden>

> ---
> Changes v1 -> v2:
> * Fix test-qdev-global-props unit test
> ---
>  hw/core/qdev-properties.c      | 27 --------------------
>  include/hw/qdev-properties.h   |  1 -
>  tests/test-qdev-global-props.c | 57
> +++---------------------------------------
> vl.c                           |  1 - 4 files changed, 4
> insertions(+), 82 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 0fe7214..c14791d 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1036,33 +1036,6 @@ void
> qdev_prop_register_global_list(GlobalProperty *props) }
>  }
>  
> -int qdev_prop_check_globals(void)
> -{
> -    GList *l;
> -    int ret = 0;
> -
> -    for (l = global_props; l; l = l->next) {
> -        GlobalProperty *prop = l->data;
> -        ObjectClass *oc;
> -        DeviceClass *dc;
> -        if (prop->used) {
> -            continue;
> -        }
> -        if (!prop->user_provided) {
> -            continue;
> -        }
> -        oc = object_class_by_name(prop->driver);
> -        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;
> -        }
> -    }
> -    return ret;
> -}
> -
>  static void qdev_prop_set_globals_for_type(DeviceState *dev,
>                                  const char *typename)
>  {
> diff --git a/include/hw/qdev-properties.h
> b/include/hw/qdev-properties.h index 034b75a..3bd5ea9 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -191,7 +191,6 @@ void qdev_prop_set_ptr(DeviceState *dev, const
> char *name, void *value); 
>  void qdev_prop_register_global(GlobalProperty *prop);
>  void qdev_prop_register_global_list(GlobalProperty *props);
> -int qdev_prop_check_globals(void);
>  void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret,
> DeviceState *dev, Property *prop, const char *value);
> diff --git a/tests/test-qdev-global-props.c
> b/tests/test-qdev-global-props.c index db77ad9..c0fea84 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -64,7 +64,7 @@ static const TypeInfo static_prop_type = {
>  };
>  
>  /* Test simple static property setting to default value */
> -static void test_static_prop_subprocess(void)
> +static void test_static_prop(void)
>  {
>      MyType *mt;
>  
> @@ -74,16 +74,8 @@ static void test_static_prop_subprocess(void)
>      g_assert_cmpuint(mt->prop1, ==, PROP_DEFAULT);
>  }
>  
> -static void test_static_prop(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/static/default/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr("");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  /* Test setting of static property using global properties */
> -static void test_static_globalprop_subprocess(void)
> +static void test_static_globalprop(void)
>  {
>      MyType *mt;
>      static GlobalProperty props[] = {
> @@ -100,14 +92,6 @@ static void
> test_static_globalprop_subprocess(void) g_assert_cmpuint(mt->prop2,
> ==, PROP_DEFAULT); }
>  
> -static void test_static_globalprop(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/static/global/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr("");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  #define TYPE_DYNAMIC_PROPS "dynamic-prop-type"
>  #define DYNAMIC_TYPE(obj) \
>      OBJECT_CHECK(MyType, (obj), TYPE_DYNAMIC_PROPS)
> @@ -195,7 +179,7 @@ static const TypeInfo nondevice_type = {
>  };
>  
>  /* Test setting of dynamic properties using global properties */
> -static void test_dynamic_globalprop_subprocess(void)
> +static void test_dynamic_globalprop(void)
>  {
>      MyType *mt;
>      static GlobalProperty props[] = {
> @@ -205,7 +189,6 @@ static void
> test_dynamic_globalprop_subprocess(void) { TYPE_UNUSED_NOHOTPLUG,
> "prop5", "105", true }, {}
>      };
> -    int all_used;
>  
>      qdev_prop_register_global_list(props);
>  
> @@ -214,27 +197,14 @@ static void
> test_dynamic_globalprop_subprocess(void) 
>      g_assert_cmpuint(mt->prop1, ==, 101);
>      g_assert_cmpuint(mt->prop2, ==, 102);
> -    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);
>  }
>  
> -static void test_dynamic_globalprop(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/dynamic/global/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr_unmatched("*prop1*");
> -    g_test_trap_assert_stderr_unmatched("*prop2*");
> -    g_test_trap_assert_stderr_unmatched("*prop4*");
> -    g_test_trap_assert_stderr("*Warning: global
> nohotplug-type.prop5=105 not used\n*");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  /* Test setting of dynamic properties using user_provided=false
> properties */ -static void
> test_dynamic_globalprop_nouser_subprocess(void) +static void
> test_dynamic_globalprop_nouser(void) {
>      MyType *mt;
>      static GlobalProperty props[] = {
> @@ -244,7 +214,6 @@ static void
> test_dynamic_globalprop_nouser_subprocess(void)
> { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" }, {}
>      };
> -    int all_used;
>  
>      qdev_prop_register_global_list(props);
>  
> @@ -253,22 +222,12 @@ static void
> test_dynamic_globalprop_nouser_subprocess(void) 
>      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);
>  }
>  
> -static void test_dynamic_globalprop_nouser(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess",
> 0, 0);
> -    g_test_trap_assert_passed();
> -    g_test_trap_assert_stderr("");
> -    g_test_trap_assert_stdout("");
> -}
> -
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -280,23 +239,15 @@ int main(int argc, char **argv)
>      type_register_static(&nohotplug_type);
>      type_register_static(&nondevice_type);
>  
> -    g_test_add_func("/qdev/properties/static/default/subprocess",
> -                    test_static_prop_subprocess);
>      g_test_add_func("/qdev/properties/static/default",
>                      test_static_prop);
>  
> -    g_test_add_func("/qdev/properties/static/global/subprocess",
> -                    test_static_globalprop_subprocess);
>      g_test_add_func("/qdev/properties/static/global",
>                      test_static_globalprop);
>  
> -    g_test_add_func("/qdev/properties/dynamic/global/subprocess",
> -                    test_dynamic_globalprop_subprocess);
>      g_test_add_func("/qdev/properties/dynamic/global",
>                      test_dynamic_globalprop);
>  
> -
> g_test_add_func("/qdev/properties/dynamic/global/nouser/subprocess",
> -                    test_dynamic_globalprop_nouser_subprocess);
>      g_test_add_func("/qdev/properties/dynamic/global/nouser",
>                      test_dynamic_globalprop_nouser);
>  
> diff --git a/vl.c b/vl.c
> index 8c40d56..9472a26 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4631,7 +4631,6 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> -    qdev_prop_check_globals();
>      if (vmstate_dump_file) {
>          /* dump and exit */
>          dump_vmstate_json_to_file(vmstate_dump_file);