Re: [Qemu-devel] Should not abort on -global <nonexistant dev prop>

[Eduardo Habkost]

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