object_new() cannot fail, and that's fundamental (was: [PATCH v2 1/2] qom/object, qdev: move globals functions to object.c)

[Markus Armbruster]

Daniel P. Berrangé <berrange@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é <berrange@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@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.

>       We can't do that given this need to check the 'hotplugged'
> property.
>
> That check, however, is total insanity. Pre-existing problem,
> not your fault.
>
> I imagine the rationale is that we don't want to kill QEMU
> if setting a global fails, and we're in middle of device_add
> on a running VM.

Yes.

> Throwing away errors though is unacceptable IMHO.

To be precise: we're silently ignoring any -global that fail to apply.

I agree that's wrong.

>                                                   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.fsf@pond.sub.org>
https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@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?

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.

Thoughts?