| Message ID | 1466437983-27133-8-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 20 Jun 2016 12:53:00 -0300 Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > 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);
Eduardo Habkost <ehabkost@redhat.com> writes: > 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. Because then the option will never be used. > 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. Well, there is: create the machine, look for the device. I guess what you're trying to say is that there's no way to *predict* what a machine type will instantiate, at least not until after the fact. Suggest "will instantiate a given device". > 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. Well, telling me that I gave a non-sensical option isn't always useless. I might have fat-fingered something, or be confused about my devices. Say I give -global PIIX4_PM.disable_s3=1. Does what I want with an i440FX machine type. Except it does nothing with -no-acpi or with a Q35 machine type. The former is harmless enough, but the latter is a mistake; I need to say -global ICH9-LPC.disable_s3=1. Before this patch, I get a warning in either case. Is this warning really useless? [...]
On Thu, Jun 23, 2016 at 10:17:55AM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > 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. > > Because then the option will never be used. > > > 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. > > Well, there is: create the machine, look for the device. I guess what > you're trying to say is that there's no way to *predict* what a machine > type will instantiate, at least not until after the fact. > > Suggest "will instantiate a given device". I would fix it, but I will drop the patch from the series instead (see below). > > > 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. > > Well, telling me that I gave a non-sensical option isn't always useless. > I might have fat-fingered something, or be confused about my devices. > > Say I give -global PIIX4_PM.disable_s3=1. Does what I want with an > i440FX machine type. Except it does nothing with -no-acpi or with a Q35 > machine type. The former is harmless enough, but the latter is a > mistake; I need to say -global ICH9-LPC.disable_s3=1. Before this > patch, I get a warning in either case. > > Is this warning really useless? Not entirely useless. But I was giving more importance to code simplicity than to the detection of those mistakes. But the extra fields and extra code don't bother me so much. I will drop patches 07/10 and 08/10 from the series.
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);
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 <ehabkost@redhat.com> --- 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(-)