| Message ID | 20140606201429.GK15000@otherpad.lan.raisama.net |
|---|---|
| State | New |
| Headers | show |
On 06/06/14 16:14, Eduardo Habkost wrote: > This avoids QEMU from aborting on cases like this: > > $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 > qemu-system-x86_64: Property '.foobar' not found > Aborted (core dumped) > > The code sets dev->not_used if the property is not found as an effort to > to allow errors to be reported even if the device is hotpluggable, but > it won't catch all errors. We can't know the property is not going to be > available for hotpluggable devices, unless we actually try to create the > device. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/qdev-properties.c | 10 +++++++++- > tests/test-qdev-global-props.c | 14 ++++++++++++-- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 3d12560..8cd7c2a 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -976,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > Error **errp) > { > GlobalProperty *prop; > + Object *obj = OBJECT(dev); > > QTAILQ_FOREACH(prop, &global_props, next) { > Error *err = NULL; > @@ -983,8 +984,15 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > if (strcmp(typename, prop->driver) != 0) { > continue; > } > + if (!object_property_find(obj, prop->property, &err)) { > + /* not_used doesn't default to true for hotpluggable devices, > + * but in this case we know the property wasn't used when it could. > + */ > + prop->not_used = true; > + continue; > + } > prop->not_used = false; > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err); > + object_property_parse(obj, prop->value, prop->property, &err); > if (err != NULL) { > error_propagate(errp, err); > return; > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c > index 2bef04c..1ccc3e5 100644 > --- a/tests/test-qdev-global-props.c > +++ b/tests/test-qdev-global-props.c > @@ -148,8 +148,9 @@ static void test_dynamic_globalprop(void) > { > MyType *mt; > static GlobalProperty props[] = { > - { TYPE_DYNAMIC_PROPS, "prop1", "101" }, > - { TYPE_DYNAMIC_PROPS, "prop2", "102" }, > + { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, > + { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, > + { TYPE_DYNAMIC_PROPS, "prop3", "103", true }, I think the test would be better if this started out as false. -Don Slutz > { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, > {} > }; > @@ -164,6 +165,15 @@ static void test_dynamic_globalprop(void) > g_assert_cmpuint(mt->prop2, ==, 102); > all_used = qdev_prop_check_global(); > g_assert_cmpuint(all_used, ==, 1); > + > + /* prop1 */ > + g_assert(!props[0].not_used); > + /* prop2 */ > + g_assert(!props[1].not_used); > + /* TYPE_DYNAMIC_PROPS.prop3: non-existing property */ > + g_assert(props[2].not_used); > + /* TYPE_DYNAMIC_PROPS-bad.prop3: non-existing class */ > + g_assert(props[3].not_used); > } > > int main(int argc, char **argv)
On Fri, 6 Jun 2014 17:14:29 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > This avoids QEMU from aborting on cases like this: > > $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 > qemu-system-x86_64: Property '.foobar' not found > Aborted (core dumped) That is expected behavior. > > The code sets dev->not_used if the property is not found as an effort to > to allow errors to be reported even if the device is hotpluggable, but > it won't catch all errors. We can't know the property is not going to be > available for hotpluggable devices, unless we actually try to create the > device. Instead of ignoring users errors, DeviceState should have async_error field which could be set by device_post_init() instead of aborting and later device_add could gracefully fail hotadd operation if error is set. PS: initfn-s could also reuse this, instead of ignoring errors as they do now. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/qdev-properties.c | 10 +++++++++- > tests/test-qdev-global-props.c | 14 ++++++++++++-- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 3d12560..8cd7c2a 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -976,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > Error **errp) > { > GlobalProperty *prop; > + Object *obj = OBJECT(dev); > > QTAILQ_FOREACH(prop, &global_props, next) { > Error *err = NULL; > @@ -983,8 +984,15 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, > if (strcmp(typename, prop->driver) != 0) { > continue; > } > + if (!object_property_find(obj, prop->property, &err)) { > + /* not_used doesn't default to true for hotpluggable devices, > + * but in this case we know the property wasn't used when it could. > + */ > + prop->not_used = true; > + continue; > + } > prop->not_used = false; > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err); > + object_property_parse(obj, prop->value, prop->property, &err); > if (err != NULL) { > error_propagate(errp, err); > return; > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c > index 2bef04c..1ccc3e5 100644 > --- a/tests/test-qdev-global-props.c > +++ b/tests/test-qdev-global-props.c > @@ -148,8 +148,9 @@ static void test_dynamic_globalprop(void) > { > MyType *mt; > static GlobalProperty props[] = { > - { TYPE_DYNAMIC_PROPS, "prop1", "101" }, > - { TYPE_DYNAMIC_PROPS, "prop2", "102" }, > + { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, > + { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, > + { TYPE_DYNAMIC_PROPS, "prop3", "103", true }, > { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, > {} > }; > @@ -164,6 +165,15 @@ static void test_dynamic_globalprop(void) > g_assert_cmpuint(mt->prop2, ==, 102); > all_used = qdev_prop_check_global(); > g_assert_cmpuint(all_used, ==, 1); > + > + /* prop1 */ > + g_assert(!props[0].not_used); > + /* prop2 */ > + g_assert(!props[1].not_used); > + /* TYPE_DYNAMIC_PROPS.prop3: non-existing property */ > + g_assert(props[2].not_used); > + /* TYPE_DYNAMIC_PROPS-bad.prop3: non-existing class */ > + g_assert(props[3].not_used); > } > > int main(int argc, char **argv) > -- > 1.9.0 > >
On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote: > On Fri, 6 Jun 2014 17:14:29 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > This avoids QEMU from aborting on cases like this: > > > > $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 > > qemu-system-x86_64: Property '.foobar' not found > > Aborted (core dumped) > That is expected behavior. Why? QEMU should never dump core due to user error. QEMU should not abort when handling a device_add command due to user error. > > > > > The code sets dev->not_used if the property is not found as an effort to > > to allow errors to be reported even if the device is hotpluggable, but > > it won't catch all errors. We can't know the property is not going to be > > available for hotpluggable devices, unless we actually try to create the > > device. > Instead of ignoring users errors, DeviceState should have async_error > field which could be set by device_post_init() instead of aborting and > later device_add could gracefully fail hotadd operation if error is set. > > PS: > initfn-s could also reuse this, instead of ignoring errors as they do now. Your proposal sounds good, and would allow reporting error without creating an object_new() variation that accepts Error**. But I believe we need to choose what to do in the meantime, while we don't have that mechanism implemented. Dumping core is not acceptable. Exiting QEMU while handling device_add doesn't seem acceptable to me, either.
On Fri, 6 Jun 2014 19:21:36 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote: > > On Fri, 6 Jun 2014 17:14:29 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > This avoids QEMU from aborting on cases like this: > > > > > > $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 > > > qemu-system-x86_64: Property '.foobar' not found > > > Aborted (core dumped) > > That is expected behavior. > > Why? > > QEMU should never dump core due to user error. > > QEMU should not abort when handling a device_add command due to user > error. I've meant QEMU shouldn't start if CLI has error. whether it's abort or exit(FAIL) doesn't matter much. > > > > > > > > > The code sets dev->not_used if the property is not found as an effort to > > > to allow errors to be reported even if the device is hotpluggable, but > > > it won't catch all errors. We can't know the property is not going to be > > > available for hotpluggable devices, unless we actually try to create the > > > device. > > Instead of ignoring users errors, DeviceState should have async_error > > field which could be set by device_post_init() instead of aborting and > > later device_add could gracefully fail hotadd operation if error is set. > > > > PS: > > initfn-s could also reuse this, instead of ignoring errors as they do now. > > Your proposal sounds good, and would allow reporting error without > creating an object_new() variation that accepts Error**. > > But I believe we need to choose what to do in the meantime, while we > don't have that mechanism implemented. Dumping core is not acceptable. > Exiting QEMU while handling device_add doesn't seem acceptable to me, > either. Exiting at startup is fine and allows to filter out user errors early. During hotplug exiting is certainly not an option, that's why I'm suggesting add async_error so that hotplug operation could fail gracefully. It's a cleaner approach and not much more complex. Ignoring errors on the other side would introduce relaxed CLI interface that we would have to support forever for compatibility reasons. > > -- > Eduardo
On 7 June 2014 00:22, Igor Mammedov <imammedo@redhat.com> wrote: > Eduardo Habkost <ehabkost@redhat.com> wrote: >> On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote: >> > Eduardo Habkost <ehabkost@redhat.com> wrote: >> > > $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 >> > > qemu-system-x86_64: Property '.foobar' not found >> > > Aborted (core dumped) >> > That is expected behavior. >> >> Why? >> >> QEMU should never dump core due to user error. >> >> QEMU should not abort when handling a device_add command due to user >> error. > I've meant QEMU shouldn't start if CLI has error. whether it's abort or > exit(FAIL) doesn't matter much. I'm with Eduardo on this one -- if the user passes us a bad command line we should diagnose it helpfully and exit with a failure code; abort() is for programming errors. (If nothing else, using abort() for user-triggerable conditions tends to mean we get bug reports about core dumps, so it's in our own interest to not do that :-) ) thanks -- PMM
On Sat, Jun 07, 2014 at 12:45:26AM +0100, Peter Maydell wrote: > On 7 June 2014 00:22, Igor Mammedov <imammedo@redhat.com> wrote: > > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> On Fri, Jun 06, 2014 at 11:38:58PM +0200, Igor Mammedov wrote: > >> > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > > $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 > >> > > qemu-system-x86_64: Property '.foobar' not found > >> > > Aborted (core dumped) > >> > That is expected behavior. > >> > >> Why? > >> > >> QEMU should never dump core due to user error. > >> > >> QEMU should not abort when handling a device_add command due to user > >> error. > > I've meant QEMU shouldn't start if CLI has error. whether it's abort or > > exit(FAIL) doesn't matter much. > > I'm with Eduardo on this one -- if the user passes us a bad > command line we should diagnose it helpfully and exit with > a failure code; abort() is for programming errors. (If nothing > else, using abort() for user-triggerable conditions tends to > mean we get bug reports about core dumps, so it's in our > own interest to not do that :-) ) So you are agreeing with Igor, as well. He is just suggesting we keep terminating QEMU on that case, and we can use exit() for that. My patch, on the other hand, makes QEMU not terminate on those cases. Note that we have two different bugs: 1) QEMU core dumps in case of user error. This is a regression introduced in 1.7.0. It can be fixed easily by changing existing code to use exit() instead of aborting. 2) QEMU terminating on device_add in case of user error. This bug was always present. It can be addressed using the async_error approach suggested by Igor. I will submit a new patch to address (1), first.
On 7 June 2014 02:09, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Sat, Jun 07, 2014 at 12:45:26AM +0100, Peter Maydell wrote: >> I'm with Eduardo on this one -- if the user passes us a bad >> command line we should diagnose it helpfully and exit with >> a failure code; abort() is for programming errors. (If nothing >> else, using abort() for user-triggerable conditions tends to >> mean we get bug reports about core dumps, so it's in our >> own interest to not do that :-) ) > > So you are agreeing with Igor, as well. He is just suggesting we keep > terminating QEMU on that case, and we can use exit() for that. My patch, > on the other hand, makes QEMU not terminate on those cases. No, I'm happy with not terminating and handling the case correctly if that makes more sense for this case; I'm just saying that dumping core is definitely not OK. thanks -- PMM
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 3d12560..8cd7c2a 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -976,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, Error **errp) { GlobalProperty *prop; + Object *obj = OBJECT(dev); QTAILQ_FOREACH(prop, &global_props, next) { Error *err = NULL; @@ -983,8 +984,15 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, if (strcmp(typename, prop->driver) != 0) { continue; } + if (!object_property_find(obj, prop->property, &err)) { + /* not_used doesn't default to true for hotpluggable devices, + * but in this case we know the property wasn't used when it could. + */ + prop->not_used = true; + continue; + } prop->not_used = false; - object_property_parse(OBJECT(dev), prop->value, prop->property, &err); + object_property_parse(obj, prop->value, prop->property, &err); if (err != NULL) { error_propagate(errp, err); return; diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index 2bef04c..1ccc3e5 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -148,8 +148,9 @@ static void test_dynamic_globalprop(void) { MyType *mt; static GlobalProperty props[] = { - { TYPE_DYNAMIC_PROPS, "prop1", "101" }, - { TYPE_DYNAMIC_PROPS, "prop2", "102" }, + { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, + { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, + { TYPE_DYNAMIC_PROPS, "prop3", "103", true }, { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, {} }; @@ -164,6 +165,15 @@ static void test_dynamic_globalprop(void) g_assert_cmpuint(mt->prop2, ==, 102); all_used = qdev_prop_check_global(); g_assert_cmpuint(all_used, ==, 1); + + /* prop1 */ + g_assert(!props[0].not_used); + /* prop2 */ + g_assert(!props[1].not_used); + /* TYPE_DYNAMIC_PROPS.prop3: non-existing property */ + g_assert(props[2].not_used); + /* TYPE_DYNAMIC_PROPS-bad.prop3: non-existing class */ + g_assert(props[3].not_used); } int main(int argc, char **argv)
This avoids QEMU from aborting on cases like this: $ ./install/bin/qemu-system-x86_64 -global cpu.foobar=5 qemu-system-x86_64: Property '.foobar' not found Aborted (core dumped) The code sets dev->not_used if the property is not found as an effort to to allow errors to be reported even if the device is hotpluggable, but it won't catch all errors. We can't know the property is not going to be available for hotpluggable devices, unless we actually try to create the device. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/core/qdev-properties.c | 10 +++++++++- tests/test-qdev-global-props.c | 14 ++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-)