On Thu, Aug 01, 2024 at 08:58:10AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berra...@redhat.com> writes:
>
> > CC: Markus since he's had opinions on stuff related to -global in
> > the past.
> >
> > On Wed, Jul 03, 2024 at 05:41:48PM -0300, Daniel Henrique Barboza wrote:
> >> Next patch will add Accel globals support. This means that globals won't be
> >> qdev exclusive logic since it'll have to deal with TYPE_ACCEL objects.
> >>
> >> Move all globals related functions and declarations to object.c. Each
> >> function is renamed from 'qdev_' to 'object_':
> >>
> >> - qdev_prop_register_global() is now object_prop_register_global()
> >> - qdev_find_global_prop() is now object_find_global_prop()
> >> - qdev_prop_check_globals() is now object_prop_check_globals()
> >> - qdev_prop_set_globals() is now object_prop_set_globals()
> >>
> >> For object_prop_set_globals() an additional change was made: the function
> >> was hardwired to be used with DeviceState, where dev->hotplugged is checked
> >> to determine if object_apply_global_props() will receive a NULL or an
> >> &error_fatal errp. The function now receives an Object and an errp, and
> >> logic using dev->hotplugged is moved to its caller (device_post_init()).
> >>
> >> Suggested-by: Daniel P. Berrangé <berra...@redhat.com>
> >> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
>
> [...]
>
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index f3a996f57d..894372b776 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -673,7 +673,7 @@ static void device_post_init(Object *obj)
> >> * precedence.
> >> */
> >> object_apply_compat_props(obj);
> >> - qdev_prop_set_globals(DEVICE(obj));
> >> + object_prop_set_globals(obj, DEVICE(obj)->hotplugged ? NULL :
> >> &error_fatal);
> >> }
> >
> > This is pretty awkward :-(
> >
> > If we're generalizing this global properties concept, then we want
> > object_prop_set_globals to be called from the Object base class
> > code.
>
> Yes. This series copies the concept from devices to accelerators. But
> since it clearly makes sense for any kind of object, we better move it
> to objects instead. We may not be able to get there in one step,
> though.
Looking at how 'DEVICE(obj)->hotplugged' gets set, this logic does
not really need to have a dependancy on Device.
static void device_initfn(Object *obj)
{
DeviceState *dev = DEVICE(obj);
if (phase_check(PHASE_MACHINE_READY)) {
dev->hotplugged = 1;
...
}
IOW, we could at least be doing
object_prop_set_globals(obj, phase_check(PHASE_MACHINE_READY) ? NULL :
&error_fatal);
if we wanted this logic in the Object base class, however....
> > device_add
> > can report errors and we should be propagating them. Likewise
> > for object_add, or any object HMP command creating QOM types.
> >
> > The trouble is that we're about 4-5 levels deep in a call
> > chain that lacks "Error **errp".
> >
> > The root problem is that none of object_new, object_new_with_class
> > and object_new_with_type have a "Error *errp" parameter.
>
> This is a fundamental QOM design decision.
>
> Not mine, mind. Moreover, I wasn't there, so my idea on design
> rationale may well be off; keep that in mind.
>
> QOM properties are not declared statically, they are created
> dynamically. Aside: this is, in my not particularly humble opinion, a
> spectacularly bad idea.
>
> Properties are generally created in instance_init() methods.
>
> Fine print: we later added "class properties", which are created
> dynamically within the class, and cloned into the instance before
> its instance_init() method runs.
>
> Object creation doesn't take arguments, and cannot fail. An
> instance_init() method doesn't take arguments, and cannot fail.
>
> Objects are configured via properties. Property setters take an
> argument (the property value), and can fail.
>
> Any part of object creation + configuration that could fail must be done
> in property setters.
>
> Common usage is create object, configure by setting properties, operate.
>
> The state transition between "configuring" and "operating" is important.
> For devices, this state transition happens when property "realized" is
> set to true. For user-creatable objects it happens when method
> complete() is called. Both can fail. For everything else, the
> transition is implicit / ad hoc / unclear.
>
> For more on this (and other QOM design issues), see my memo "Dynamic &
> heterogeneous machines, initial configuration: problems", in particular
> section "Problem 5: QOM lacks a clear life cycle".
> Message-ID: <87o7d1i7ky....@pond.sub.org>
> https://lore.kernel.org/qemu-devel/87o7d1i7ky....@pond.sub.org/
>
> To introspect properties, you need an object. You can always create one
> for that (can't fail). Properties created outside object initialization
> cannot be introspected that way. This is how qom-list-properties works.
>
> > object_new_with_props and object_new_with_propv both *do* have
> > a "Error *errp" parameter,
>
> Yes, because they combine object creation, which cannot fail, with
> setting properties, which can fail.
>
> > but then they call into object_new_with_type
> > and can't get errors back from that.
> >
> > IMHO we need to fix this inability to report errors from object
> > construction. It will certainly be a painful refactoring job,
> > but I think its neccessary in order to support global props
> > without this horrible hack checking the "hotpluggable" flag.
>
> Beyond painful. Possibly infeasible.
>
> Object creation cannot fail. If we revise this fundamental QOM design
> decision, we get to update all call chains leading to object creation.
> That's a *massive* undertaking.
>
> Can we solve the problems we have without revising QOM design?
Hmm, to cover object_add QMP and -object CLI, we could just put
setting of global props into the object_new_with_prop{s,v} method,
which would deal with usage of user creatable objects, and a few
internal calls which handle errors.
device_add/-device won't hit that code path. Even if we converted
those over to object_new_with_prop{s,v}, that only handles the top
level device. Many devices will create sub-devices behind the scenes
and global properties are supposed to apply to those, and those will
all just be using plain qdev_new().
> We'd need to delay the actual failure to a point where the design admits
> failure.
>
> Here's an idea. Formalize the life cycle, i.e. make it an explicit
> state machine. Add a "failed" state. Any error during object creation
> makes the object go to "failed". Going from "failed" to "operating"
> fails. Which is fine, because the design admits failure there.
I guess the challenge is that we then need to be confident that the
state change is effected everywhere it needs to be, which sounds like
another bit of hairy work unless we can limit the scope.
We could do
* Add globals support to object_new_with_prop{s,v}
* Convert Accel creation over to object_new_with_prop{s,v},
* Records errors setting globals in device_post_init in a
field in DeviceState
* Make qdev_realize() check for & report the error from
the step above, before doing its normal logic
IOW, going forward we have two patterns
* Creation uses object_new_with_prop{s,v} and has globals
set that way (preferred). Works for -object, -accel,
object_add and some internal direct API calls
* Creation uses a post-init hook, but the type has to have
some state change equivalent to 'realized' to do delayed
reporting of globals errors (discouraged, ideally only
for Device). Works for -device, device_add
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
