On Fri, Jun 06, 2014 at 10:01:34AM +0200, Paolo Bonzini wrote: > Il 06/06/2014 09:09, Markus Armbruster ha scritto: > >Looks like this regressed in Eduardo's commit 99a0b03 qdev: Set globals > >in instance_post_init function. > > > >Before, we exited cleanly on this error, in device_initfn(): > > > > qdev_prop_set_globals(dev, &err); > > if (err != NULL) { > > qerror_report_err(err); > > error_free(err); > > exit(1); > > } > > Hmm, right. I had noticed this abort in the past, but I wasn't sure > if it was a regression or not. > > However, the above is not hotplug-friendly either. In this sense, I > prefer an assertion that clearly says "gotta fix something here".
The assertion can be triggered by user error (a non-existing property or invalid property value). Sounds like a bug to me. I will work on a patch to get back to using exit(), except when it is just a non-existing property. It will still be hotplug-unfriendly, but abort() is not hotplug-friendly either... > > >The commit asserts qdev_prop_set_globals() can't fail, which is wrong. > > > > static void device_post_init(Object *obj) > > { > > DeviceState *dev = DEVICE(obj); > > Error *err = NULL; > > > > qdev_prop_set_globals(dev, &err); > > assert_no_error(err); > > } > > > >Later simplified to: > > > > static void device_post_init(Object *obj) > > { > > qdev_prop_set_globals(DEVICE(obj), &error_abort); > > } > > I think the bug is that the global property should have been filtered > out early. Even if we handle non-existing properties cleanly (without exiting, but just keeping prop->not_used=true), we still need to handle errors returned by property setters in a hotplug-friendly way. > Or alternatively we need something better than > device_post_init to set the globals... no ideas for now. The root problem here seems to be that property setters can report errors, but object_new() can't. Does that mean we need a variation of object_new() which can report errors? -- Eduardo