Hi
On Thu, Apr 6, 2023 at 7:00 PM Stefan Berger <stef...@linux.ibm.com> wrote:
>
>
>
> On 4/6/23 10:36, Peter Maydell wrote:
> > On Thu, 6 Apr 2023 at 15:13, Stefan Berger <stef...@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);
> - }
> }
>
looks like a fix to me, send a patch?
thanks
> 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
>
--
Marc-André Lureau
