With error_propagate, the stack trace from any error_abort/fatal
usage will start from the error_propagate() call, which is largely
useless. Using ERRP_GUARD ensures the stack trace starts from
the origin that reported the error.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qom/object_interfaces.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 1ffea1a728..415cbee8c5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -24,13 +24,12 @@
 bool user_creatable_complete(UserCreatable *uc, Error **errp)
 {
     UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc);
-    Error *err = NULL;
+    ERRP_GUARD();
 
     if (ucc->complete) {
-        ucc->complete(uc, &err);
-        error_propagate(errp, err);
+        ucc->complete(uc, errp);
     }
-    return !err;
+    return !*errp;
 }
 
 bool user_creatable_can_be_deleted(UserCreatable *uc)
-- 
2.50.1

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

> With error_propagate, the stack trace from any error_abort/fatal
> usage will start from the error_propagate() call, which is largely
> useless. Using ERRP_GUARD ensures the stack trace starts from
> the origin that reported the error.

Yes.

I've been chipping at error_propagate() uses on and off for a while.
There are hundreds left.

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

On Fri, Sep 19, 2025 at 01:30:18PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > With error_propagate, the stack trace from any error_abort/fatal
> > usage will start from the error_propagate() call, which is largely
> > useless. Using ERRP_GUARD ensures the stack trace starts from
> > the origin that reported the error.
> 
> Yes.
> 
> I've been chipping at error_propagate() uses on and off for a while.
> There are hundreds left.

Are there cases where it is still OK to use error_propagate or should
we be looking to eliminate all its usage ?


> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

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 :|

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

> On Fri, Sep 19, 2025 at 01:30:18PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > With error_propagate, the stack trace from any error_abort/fatal
>> > usage will start from the error_propagate() call, which is largely
>> > useless. Using ERRP_GUARD ensures the stack trace starts from
>> > the origin that reported the error.
>> 
>> Yes.
>> 
>> I've been chipping at error_propagate() uses on and off for a while.
>> There are hundreds left.
>
> Are there cases where it is still OK to use error_propagate or should
> we be looking to eliminate all its usage ?

The common use of error_propagate() is to propagate an error received
from a function to the caller.  This is better done with ERRP_GUARD().
qapi/error.h:

 * Call a function, receive an error from it, and pass it to the caller
 * - when the function returns a value that indicates failure, say
 *   false:
 *     if (!foo(arg, errp)) {
 *         handle the error...
 *     }
 * - when it does not, say because it is a void function:
 *     ERRP_GUARD();
 *     foo(arg, errp);
 *     if (*errp) {
 *         handle the error...
 *     }
 * More on ERRP_GUARD() below.
 *
 * Code predating ERRP_GUARD() still exists, and looks like this:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err); // deprecated
 *     }

We occasionally store errors on the heap, and use error_propagate() to move
them into an @errp argument.  qapi/error.h:

 * Pass an existing error to the caller:
 *     error_propagate(errp, err);
 * This is rarely needed.  When @err is a local variable, use of
 * ERRP_GUARD() commonly results in more readable code.

error_propagate() can also be used to accumulate errors.  This cannot be
done with ERRP_GUARD().  qapi/error.h:

 * Receive and accumulate multiple errors (first one wins):
 *     Error *err = NULL, *local_err = NULL;
 *     foo(arg, &err);
 *     bar(arg, &local_err);
 *     error_propagate(&err, local_err);
 *     if (err) {
 *         handle the error...
 *     }

Accumulating errors is commonly a bad idea.  Note that
g_propagate_error() explicitly prohibits such usage.  We deviated from
it.

The *possibility* of intentional error acculumation makes conversions to
ERRP_GUARD() harder.  Perhaps we should track down all uses of error
accumulation, then change error_propagate() to prohibit it.

[...]

On 9/19/25 12:15, Daniel P. Berrangé wrote:
> With error_propagate, the stack trace from any error_abort/fatal
> usage will start from the error_propagate() call, which is largely
> useless. Using ERRP_GUARD ensures the stack trace starts from
> the origin that reported the error.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   qom/object_interfaces.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 1ffea1a728..415cbee8c5 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -24,13 +24,12 @@
>   bool user_creatable_complete(UserCreatable *uc, Error **errp)
>   {
>       UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc);
> -    Error *err = NULL;
> +    ERRP_GUARD();
>   
>       if (ucc->complete) {
> -        ucc->complete(uc, &err);
> -        error_propagate(errp, err);
> +        ucc->complete(uc, errp);
>       }
> -    return !err;
> +    return !*errp;
>   }
>   
>   bool user_creatable_can_be_deleted(UserCreatable *uc)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo