From: Greg Kurz <groug@kaod.org>
To: Markus Armbruster <armbru@redhat.com>
Cc: fam@euphon.net, pburton@wavecomp.com, peter.maydell@linaro.org,
codyprime@gmail.com, jasowang@redhat.com,
mark.cave-ayland@ilande.co.uk, qemu-devel@nongnu.org,
kraxel@redhat.com, mreitz@redhat.com, qemu-block@nongnu.org,
quintela@redhat.com, arikalo@wavecomp.com, mst@redhat.com,
mdroth@linux.vnet.ibm.com, pasic@linux.ibm.com,
borntraeger@de.ibm.com, joel@jms.id.au,
marcandre.lureau@redhat.com, david@gibson.dropbear.id.au,
farman@linux.ibm.com, ehabkost@redhat.com, sw@weilnetz.de,
dgilbert@redhat.com, yuval.shaia@oracle.com,
alex.williamson@redhat.com, integration@gluster.org,
clg@kaod.org, stefanha@redhat.com, david@redhat.com,
jsnow@redhat.com, rth@twiddle.net, kwolf@redhat.com,
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
berrange@redhat.com, andrew@aj.id.au, cohuck@redhat.com,
qemu-s390x@nongnu.org, sundeep.lkml@gmail.com,
qemu-arm@nongnu.org, qemu-ppc@nongnu.org, pbonzini@redhat.com
Subject: Re: [PATCH v4 04/31] error: auto propagated local_err
Date: Tue, 8 Oct 2019 18:19:21 +0200 [thread overview]
Message-ID: <20191008181921.288e69e4@bahia.lan> (raw)
In-Reply-To: <87k19fgr7i.fsf@dusky.pond.sub.org>
On Tue, 08 Oct 2019 18:03:13 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
> > Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> > functions with errp OUT parameter.
> >
> > It has three goals:
> >
> > 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> > can't see this additional information, because exit() happens in
> > error_setg earlier than information is added. [Reported by Greg Kurz]
> >
> > 2. Fix issue with error_abort & error_propagate: when we wrap
> > error_abort by local_err+error_propagate, resulting coredump will
> > refer to error_propagate and not to the place where error happened.
> > (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> > local_err+error_propagate pattern, which will definitely fix the issue)
> > [Reported by Kevin Wolf]
> >
> > 3. Drop local_err+error_propagate pattern, which is used to workaround
> > void functions with errp parameter, when caller wants to know resulting
> > status. (Note: actually these functions could be merely updated to
> > return int error code).
>
> Starting with stating your goals is an excellent idea. But I'd love to
> next read a high-level description of how your patch achieves or enables
> achieving these goals.
>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> [...]
> > diff --git a/include/qapi/error.h b/include/qapi/error.h
> > index 9376f59c35..02f967ac1d 100644
> > --- a/include/qapi/error.h
> > +++ b/include/qapi/error.h
> > @@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
> > ErrorClass err_class, const char *fmt, ...)
> > GCC_FMT_ATTR(6, 7);
> >
> > +typedef struct ErrorPropagator {
> > + Error *local_err;
> > + Error **errp;
> > +} ErrorPropagator;
> > +
> > +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> > +{
> > + error_propagate(prop->errp, prop->local_err);
> > +}
> > +
> > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> > +
> > +/*
> > + * ERRP_AUTO_PROPAGATE
> > + *
> > + * This macro is created to be the first line of a function with Error **errp
> > + * OUT parameter. It's needed only in cases where we want to use error_prepend,
> > + * error_append_hint or dereference *errp. It's still safe (but useless) in
> > + * other cases.
> > + *
> > + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> > + * local Error object, which will be automatically propagated to the original
> > + * errp on function exit (see error_propagator_cleanup).
> > + *
> > + * After invocation of this macro it is always safe to dereference errp
> > + * (as it's not NULL anymore) and to append hints (by error_append_hint)
> > + * (as, if it was error_fatal, we swapped it with a local_error to be
> > + * propagated on cleanup).
>
> Well, appending hints was always safe, it just didn't work with
> &error_fatal. Don't worry about that now, I'll probably want to polish
> this contract comment a bit anyway, but later.
>
FWIW I've already posted this:
Author: Greg Kurz <groug@kaod.org>
Date: Mon Oct 7 15:45:46 2019 +0200
error: Update error_append_hint()'s documenation
error_setg() and error_propagate(), as well as their variants, cause
QEMU to terminate when called with &error_fatal or &error_abort. This
prevents to add hints since error_append_hint() isn't even called in
this case.
It means that error_append_hint() should only be used with a local
error object, and then propagate this local error to the caller.
Document this in <qapi/error.h> .
Signed-off-by: Greg Kurz <groug@kaod.org>
Message-id: <156871563702.196432.5964411202152101367.stgit@bahia.lan>
https://patchwork.ozlabs.org/patch/1163278/
> > + *
> > + * Note: we don't wrap the error_abort case, as we want resulting coredump
> > + * to point to the place where the error happened, not to error_propagate.
> > + */
> > +#define ERRP_AUTO_PROPAGATE() \
> > +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
>
> Took me a second to realize: the macro works, because the initializer
> implicitly initializes .local_error = NULL.
>
> __auto_errp_prop is an identifier reserved for any use. I think we
> could use _auto_errp_prop, which is only reserved for use as identifiers
> with file scope in both the ordinary and tag name spaces. See ISO/IEC
> 9899:1999 7.1.3 Reserved identifiers.
>
> > +errp = ((errp == NULL || *errp == error_fatal) ? \
> > + &__auto_errp_prop.local_err : errp)
> > +
>
> Please indent multi-line macros like elsewhere in this file:
>
> #define ERRP_AUTO_PROPAGATE() \
> g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> errp = ((errp == NULL || *errp == error_fatal) \
> ? &__auto_errp_prop.local_err : errp)
>
> > /*
> > * Special error destination to abort on error.
> > * See error_setg() and error_propagate() for details.
>
> To be honest, the cover letter left me a bit skeptical, but now I think
> you might be up to something. Let's see how the patches putting the
> macro to use come out.
next prev parent reply other threads:[~2019-10-08 16:21 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-01 15:52 [PATCH v4 00/31] error: auto propagated local_err Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument Vladimir Sementsov-Ogievskiy
2019-10-08 9:08 ` Markus Armbruster
2019-10-08 9:30 ` Vladimir Sementsov-Ogievskiy
2019-10-08 12:05 ` Markus Armbruster
2019-10-09 10:08 ` Vladimir Sementsov-Ogievskiy
2019-10-09 18:48 ` Markus Armbruster
2019-10-09 9:42 ` Vladimir Sementsov-Ogievskiy
2019-10-09 18:51 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 02/31] hw/core/loader-fit: fix freeing errp in fit_load_fdt Vladimir Sementsov-Ogievskiy
2019-10-01 16:13 ` Eric Blake
2019-10-08 14:24 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 03/31] net/net: fix local variable shadowing in net_client_init Vladimir Sementsov-Ogievskiy
2019-10-08 14:34 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 04/31] error: auto propagated local_err Vladimir Sementsov-Ogievskiy
2019-10-01 16:17 ` Eric Blake
2019-10-02 10:15 ` Roman Kagan
2019-10-02 14:00 ` Eric Blake
2019-10-08 16:03 ` Markus Armbruster
2019-10-08 16:19 ` Greg Kurz [this message]
2019-10-08 18:24 ` Markus Armbruster
2019-10-09 8:04 ` Markus Armbruster
2019-10-09 8:17 ` Vladimir Sementsov-Ogievskiy
2019-10-09 19:09 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage Vladimir Sementsov-Ogievskiy
2019-10-01 16:22 ` Eric Blake
2019-10-01 17:01 ` Vladimir Sementsov-Ogievskiy
2019-10-01 16:50 ` Eric Blake
2019-10-01 17:08 ` Eric Blake
2019-10-01 17:15 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 06/31] python: add commit-per-subsystem.py Vladimir Sementsov-Ogievskiy
2019-10-07 15:55 ` Cornelia Huck
2019-10-07 16:10 ` Vladimir Sementsov-Ogievskiy
2019-10-07 16:16 ` Cornelia Huck
2019-10-07 16:21 ` Daniel P. Berrangé
2019-10-07 17:15 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 07/31] s390: Fix error_append_hint/error_prepend usage Vladimir Sementsov-Ogievskiy
2019-10-07 15:58 ` Cornelia Huck
2019-10-09 7:42 ` Markus Armbruster
2019-10-11 15:33 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 08/31] ARM TCG CPUs: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 09/31] PowerPC " Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 10/31] arm: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 11/31] SmartFusion2: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 12/31] ASPEED BMCs: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 13/31] Boston: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 14/31] PowerNV (Non-Virtualized): " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 15/31] PCI: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 16/31] SCSI: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 17/31] USB: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 18/31] VFIO: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 19/31] vhost: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 20/31] virtio: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 21/31] virtio-9p: " Vladimir Sementsov-Ogievskiy
2019-10-02 9:19 ` Greg Kurz
2019-10-02 12:58 ` Vladimir Sementsov-Ogievskiy
2019-10-02 13:11 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 22/31] XIVE: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 23/31] block: " Vladimir Sementsov-Ogievskiy
2019-10-01 17:09 ` Eric Blake
2019-10-01 18:55 ` Vladimir Sementsov-Ogievskiy
2019-10-01 19:12 ` Vladimir Sementsov-Ogievskiy
2019-10-01 19:44 ` Eric Blake
2019-10-09 7:22 ` Markus Armbruster
2019-10-01 15:53 ` [PATCH v4 24/31] chardev: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 25/31] cmdline: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 26/31] QOM: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 27/31] Migration: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 28/31] Sockets: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 29/31] nbd: " Vladimir Sementsov-Ogievskiy
2019-10-01 17:47 ` Eric Blake
2019-10-01 15:53 ` [PATCH v4 30/31] PVRDMA: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 31/31] ivshmem: " Vladimir Sementsov-Ogievskiy
2019-10-02 3:26 ` [PATCH v4 00/31] error: auto propagated local_err no-reply
2019-10-02 11:58 ` Markus Armbruster
2019-10-08 7:30 ` Markus Armbruster
2019-10-08 8:41 ` Vladimir Sementsov-Ogievskiy
2019-10-08 9:39 ` Greg Kurz
2019-10-08 10:09 ` Vladimir Sementsov-Ogievskiy
2019-10-08 11:59 ` Markus Armbruster
2019-10-09 8:45 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191008181921.288e69e4@bahia.lan \
--to=groug@kaod.org \
--cc=alex.williamson@redhat.com \
--cc=andrew@aj.id.au \
--cc=arikalo@wavecomp.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=clg@kaod.org \
--cc=codyprime@gmail.com \
--cc=cohuck@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=david@redhat.com \
--cc=dgilbert@redhat.com \
--cc=ehabkost@redhat.com \
--cc=fam@euphon.net \
--cc=farman@linux.ibm.com \
--cc=integration@gluster.org \
--cc=jasowang@redhat.com \
--cc=joel@jms.id.au \
--cc=jsnow@redhat.com \
--cc=kraxel@redhat.com \
--cc=kwolf@redhat.com \
--cc=marcandre.lureau@redhat.com \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=mdroth@linux.vnet.ibm.com \
--cc=mreitz@redhat.com \
--cc=mst@redhat.com \
--cc=pasic@linux.ibm.com \
--cc=pbonzini@redhat.com \
--cc=pburton@wavecomp.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=qemu-s390x@nongnu.org \
--cc=quintela@redhat.com \
--cc=rth@twiddle.net \
--cc=stefanha@redhat.com \
--cc=sundeep.lkml@gmail.com \
--cc=sw@weilnetz.de \
--cc=vsementsov@virtuozzo.com \
--cc=yuval.shaia@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).