Re: [PULL v2 05/25] error: add global &error_warn destination

[Stefan Berger]

On 4/6/23 10:36, Peter Maydell wrote:
> On Thu, 6 Apr 2023 at 15:13, Stefan Berger <stefanb@linux.ibm.com> wrote:
> > I'll be out starting tomorrow. I don't see Marc-André online.
> >
> > Would this be acceptable?
> > It ensures that if error_handle() returns, err has been freed.
> > In the other two cases a copy is being made of the Error that can then be used 
> > after the error_handle() call.
>
> "Not error_warn" is the common case, so it doesn't seem
> great to copy the error around like that. My thoughts were
> either:
>   (1) error_handle() should handle all of the error cases,
> like this:
>
>      if (errp == &error_abort) {
>         ...
>         abort();
>      }
>      if (errp == &error_fatal) {
>         ...
>         exit(1);
>      }
>      if (errp = &error_warn) {
>          warn_report_err(err);
>      } else if (errp && !*errp) {
>          *errp = err;
>      } else {
>          error_free(err);
>      }
>
> and delete the "set *errp" logic from the callers.


Like this?

diff --git a/util/error.c b/util/error.c
index 5537245da6..e5e247209a 100644
--- a/util/error.c
+++ b/util/error.c
@@ -46,6 +46,10 @@ static void error_handle(Error **errp, Error *err)
     }
     if (errp == &error_warn) {
         warn_report_err(err);
+    } else if (errp && !*errp) {
+        *errp = err;
+    } else {
+        error_free(err);
     }
 }

@@ -76,7 +80,6 @@ static void error_setv(Error **errp,
     err->func = func;

     error_handle(errp, err);
-    *errp = err;

     errno = saved_errno;
 }
@@ -289,11 +292,6 @@ void error_propagate(Error **dst_errp, Error *local_err)
         return;
     }
     error_handle(dst_errp, local_err);
-    if (dst_errp && !*dst_errp) {
-        *dst_errp = local_err;
-    } else {
-        error_free(local_err);
-    }
 }

 void error_propagate_prepend(Error **dst_errp, Error *err,



> (note that error_setv() has done checks already that mean it
> will always take the "(errp && !*errp)" path, so we don't need
> to special-case for which caller this was.)
>
> (2) error_handle() should return a bool to say whether it's handled
> the error entirely and the callsite should do nothing further

> with it.
>
> I prefer (1) I think but would defer to people with more
> experience with the Error APIs.
>
> thanks
> -- PMM