Re: [Qemu-devel] [PATCH v2 03/10] vl: Reject invalid class names on -global

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> Instead of just printing a warning very late, reject obviously
> invalid -global arguments by validating the class name.
>
> Update test-qdev-global-props to not expect class name validation
> to be performed in qdev_prop_check_globals().
>
> Reviewed-by: Markus Armbruster <address@hidden>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Changes v1 -> v2:
> * Fix test-qdev-global-props unit test
> * Simplify object_dynamic_cast() check
>   * Suggested-by: Markus Armbruster <address@hidden>
> ---
>  hw/core/qdev-properties.c      |  7 -------
>  tests/test-qdev-global-props.c | 10 ----------
>  vl.c                           | 20 +++++++++++++++++---
>  3 files changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index c10edee..64e17aa 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1052,13 +1052,6 @@ int qdev_prop_check_globals(void)
>              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",
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 48e5b73..db77ad9 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -201,10 +201,8 @@ static void test_dynamic_globalprop_subprocess(void)
>      static GlobalProperty props[] = {
>          { TYPE_DYNAMIC_PROPS, "prop1", "101", true },
>          { TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
>          { TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
>          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
> -        { TYPE_NONDEVICE, "prop6", "106", true },
>          {}
>      };
>      int all_used;
> @@ -222,8 +220,6 @@ static void test_dynamic_globalprop_subprocess(void)
>      g_assert(props[1].used);
>      g_assert(!props[2].used);
>      g_assert(!props[3].used);
> -    g_assert(!props[4].used);
> -    g_assert(!props[5].used);
>  }
>  
>  static void test_dynamic_globalprop(void)
> @@ -232,10 +228,8 @@ static void test_dynamic_globalprop(void)
>      g_test_trap_assert_passed();
>      g_test_trap_assert_stderr_unmatched("*prop1*");
>      g_test_trap_assert_stderr_unmatched("*prop2*");
> -    g_test_trap_assert_stderr("*Warning: global dynamic-prop-type-bad.prop3 
> has invalid class name\n*");
>      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_stderr("*Warning: global nondevice-type.prop6 has 
> invalid class name\n*");
>      g_test_trap_assert_stdout("");
>  }
>  
> @@ -246,10 +240,8 @@ static void 
> test_dynamic_globalprop_nouser_subprocess(void)
>      static GlobalProperty props[] = {
>          { TYPE_DYNAMIC_PROPS, "prop1", "101" },
>          { TYPE_DYNAMIC_PROPS, "prop2", "102" },
> -        { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" },
>          { TYPE_UNUSED_HOTPLUG, "prop4", "104" },
>          { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
> -        { TYPE_NONDEVICE, "prop6", "106" },
>          {}
>      };
>      int all_used;
> @@ -267,8 +259,6 @@ static void 
> test_dynamic_globalprop_nouser_subprocess(void)
>      g_assert(props[1].used);
>      g_assert(!props[2].used);
>      g_assert(!props[3].used);
> -    g_assert(!props[4].used);
> -    g_assert(!props[5].used);
>  }
>  

The rest of the patch turns a warning into an error, but the test update
*drops* the affected test cases.  Shouldn't we test the error instead?

To keep my R-by, you can either do that, or mention the loss of test
cases in the commit message.

[...]