Daniel P. Berrangé <[email protected]> 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 <[email protected]>
> Tested-by: Peter Krempa <[email protected]>
> Signed-off-by: Daniel P. Berrangé <[email protected]>
[...]
> 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;
>
> 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");
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.
> return false;
> }
>
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index f96610f510..e161bcfda9 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -801,7 +801,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
> # Now group1 is in use, it cannot be deleted
> result = self.vm.qmp('object-del', id = 'group1')
> self.assert_qmp(result, 'error/class', 'GenericError')
> - self.assert_qmp(result, 'error/desc', "object 'group1' is in use,
> can not be deleted")
> + self.assert_qmp(result, 'error/desc', "Throttle group still has
> multiple references")
>
> # Default options, this switches the group back to group0
> self.reopen(opts)
> @@ -809,7 +809,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
> # So now we cannot delete group0
> result = self.vm.qmp('object-del', id = 'group0')
> self.assert_qmp(result, 'error/class', 'GenericError')
> - self.assert_qmp(result, 'error/desc', "object 'group0' is in use,
> can not be deleted")
> + self.assert_qmp(result, 'error/desc', "Throttle group still has
> multiple references")
>
> # But group1 is free this time, and it can be deleted
> self.vm.cmd('object-del', id = 'group1')
> diff --git a/util/main-loop.c b/util/main-loop.c
> index ad8645c30a..67ee06c311 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -218,8 +218,9 @@ static void main_loop_init(EventLoopBase *base, Error
> **errp)
> mloop = m;
> }
>
> -static bool main_loop_can_be_deleted(EventLoopBase *base)
> +static bool main_loop_prepare_delete(EventLoopBase *base, Error **errp)
> {
> + error_setg(errp, "Deleting main loop is not supported");
> return false;
> }
>
> @@ -229,7 +230,7 @@ static void main_loop_class_init(ObjectClass *oc, const
> void *class_data)
>
> bc->init = main_loop_init;
> bc->update_params = main_loop_update_params;
> - bc->can_be_deleted = main_loop_can_be_deleted;
> + bc->prepare_delete = main_loop_prepare_delete;
> }
>
> static const TypeInfo main_loop_info = {
