Eduardo Habkost <ehabk...@redhat.com> 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 <arm...@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> ---
> Changes v1 -> v2:
> * Fix test-qdev-global-props unit test
> * Simplify object_dynamic_cast() check
> * Suggested-by: Markus Armbruster <arm...@redhat.com>
> ---
> 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.
[...]
