| Message ID | 1466437983-27133-4-git-send-email-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eduardo Habkost <ehabkost@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 <armbru@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v1 -> v2: > * Fix test-qdev-global-props unit test > * Simplify object_dynamic_cast() check > * Suggested-by: Markus Armbruster <armbru@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. [...]
On Thu, Jun 23, 2016 at 09:52:12AM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@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 <armbru@redhat.com> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > Changes v1 -> v2: > > * Fix test-qdev-global-props unit test > > * Simplify object_dynamic_cast() check > > * Suggested-by: Markus Armbruster <armbru@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? We could test the error, but good luck linking vl.o in a unit test. > > To keep my R-by, you can either do that, or mention the loss of test > cases in the commit message. I will mention it in the commit message. Thanks!
On Mon, Jun 20, 2016 at 12:52:56PM -0300, Eduardo Habkost wrote: > 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(). Added the following to the commit message: The checks for the warnings in tests/test-qdev-global-props.c are being removed. Unfortunately, adding unit tests for vl.c would be quite difficult. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v1 -> v2: > * Fix test-qdev-global-props unit test > * Simplify object_dynamic_cast() check > * Suggested-by: Markus Armbruster <armbru@redhat.com>
On Thu, Jun 23, 2016 at 08:40:41AM -0300, Eduardo Habkost wrote: > On Thu, Jun 23, 2016 at 09:52:12AM +0200, Markus Armbruster wrote: > > Eduardo Habkost <ehabkost@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 <armbru@redhat.com> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > Changes v1 -> v2: > > > * Fix test-qdev-global-props unit test > > > * Simplify object_dynamic_cast() check > > > * Suggested-by: Markus Armbruster <armbru@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? > > We could test the error, but good luck linking vl.o in a unit > test. > > > > > To keep my R-by, you can either do that, or mention the loss of test > > cases in the commit message. > > I will mention it in the commit message. Thanks! As I am removing the patches that remove qdev_prop_check_globals() and the used/user_provided fields, I will change the patch to just add the extra check in vl.c, and not touch the test code and qdev_prop_check_globals(). I will send v3 soon.
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); } static void test_dynamic_globalprop_nouser(void) diff --git a/vl.c b/vl.c index e9ca733..1721a4b 100644 --- a/vl.c +++ b/vl.c @@ -2931,10 +2931,20 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size, static int global_init_func(void *opaque, QemuOpts *opts, Error **errp) { GlobalProperty *g; + ObjectClass *oc; + const char *driver = qemu_opt_get(opts, "driver"); + const char *prop = qemu_opt_get(opts, "property"); + + oc = object_class_by_name(driver); + if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) { + error_setg(errp, "global %s.%s has invalid class name", + driver, prop); + return -1; + } g = g_malloc0(sizeof(*g)); - g->driver = qemu_opt_get(opts, "driver"); - g->property = qemu_opt_get(opts, "property"); + g->driver = driver; + g->property = prop; g->value = qemu_opt_get(opts, "value"); g->user_provided = true; qdev_prop_register_global(g); @@ -4487,7 +4497,11 @@ int main(int argc, char **argv, char **envp) } } qemu_opts_foreach(qemu_find_opts("global"), - global_init_func, NULL, NULL); + global_init_func, NULL, &err); + if (err) { + error_report_err(err); + exit(1); + } /* This checkpoint is required by replay to separate prior clock reading from the other reads, because timer polling functions query