Re: [PATCH v5 01/35] qom: replace 'can_be_deleted' with 'prepare_delete'

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jun 30, 2026 at 09:34:26AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > While most objects can perform all their cleanup in the finalizer
>> > method, there can be interactions with other resources / subsystems
>> > / threads which require that some cleanup be performed on an user
>> > creatable object before unparenting it and entering finalization.
>> >
>> > The current 'can_be_deleted' method runs in the deletion path and
>> > is intended to be used to block deletion. While it could be used
>> > to perform cleanup tasks, its name suggests it should be free of
>> > side-effects.
>> >
>> > Generalize this by renaming it to 'prepare_delete', explicitly
>> > allowing for cleanup to be provided. Existing users of 'can_be_deleted'
>> > are re-written, which provides them with more detailed/tailored error
>> > messages.
>> >
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Tested-by: Peter Krempa <pkrempa@redhat.com>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> 
>> [...]
>> 
>> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> > index 7080f85f95..6faa0b2fd9 100644
>> > --- a/qom/object_interfaces.c
>> > +++ b/qom/object_interfaces.c
>> > @@ -32,16 +32,15 @@ bool user_creatable_complete(UserCreatable *uc, Error **errp)
>> >      return !*errp;
>> >  }
>> >  
>> > -bool user_creatable_can_be_deleted(UserCreatable *uc)
>> > +bool user_creatable_prepare_delete(UserCreatable *uc, Error **errp)
>> >  {
>> > -
>> >      UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc);
>> > +    ERRP_GUARD();
>> >  
>> > -    if (ucc->can_be_deleted) {
>> > -        return ucc->can_be_deleted(uc);
>> > -    } else {
>> > -        return true;
>> > +    if (ucc->prepare_delete) {
>> > +        ucc->prepare_delete(uc, errp);
>> >      }
>> > +    return !*errp;
>> >  }
>> 
>> Simpler:
>> 
>>        if (ucc->prepare_delete) {
>>            return ucc->prepare_delete(uc, errp);
>>        }
>> 
>>        return true;
>
> ok
>
>> 
>> >  
>> >  void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
>> > @@ -253,8 +252,7 @@ bool user_creatable_del(const char *id, Error **errp)
>> >          return false;
>> >      }
>> >  
>> > -    if (!user_creatable_can_be_deleted(USER_CREATABLE(obj))) {
>> > -        error_setg(errp, "object '%s' is in use, can not be deleted", id);
>> > +    if (!user_creatable_prepare_delete(USER_CREATABLE(obj), errp)) {
>> 
>> This changes error messages.
>> 
>> The old ones mention @id.
>> 
>> The new ones don't:
>> 
>>         error_setg(errp, "Cryptodev backend is still in use");
>>         error_setg(errp, "Host memory backend is still mapped");
>>         error_setg(errp, "IOMMUFD backend still has %d users", be->users);
>>         error_setg(errp, "Throttle group still has multiple references");
>>     error_setg(errp, "Deleting can bus devices is not supported");

Spel it CAN, please.

>>     error_setg(errp, "Deleting main loop is not supported");
>> 
>> Visible in the diff to tests/qemu-iotests/245 below.
>> 
>> Because this runs within object-del, and @id is the argument provided by
>> the user there, the error message is still sufficiently clear, I guess.
>> 
>> If this ever gets used where the object isn't obvious to the user from
>> context, the error messages become sub-par.  Observation, not a demand.
>> 
>> Please cover this change in the commit message.
>
> Oh yes, I remember being slightly annoyed at the need to do
> this - I was surprised to realize that an Object does not
> actually know its own 'id'. The 'id' only exists as the
> name of the property in the parent.
>
> We could add an  object_get_id() API which traveres obj->parent
> and then iterates over child properties. Annoyingly O(n) time
> for that, but perhaps not a big enough problem to worry about ?
> At least for error reporting we're probably ok with that time
> complexity even if there are 1000 objects registered as children.

Could you use object_get_canonical_path_component()?

[...]