* [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort
@ 2015-06-16 12:53 Michael S. Tsirkin
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons Michael S. Tsirkin
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-16 12:53 UTC (permalink / raw
To: qemu-devel; +Cc: kwolf, armbru, dgilbert
It's a common idiom:
Error *local_err = NULL;
....
foo(&local_err);
...
if (local_err) {
error_propagate(errp, local_err);
return;
}
Unfortunately it means that call to foo(&local_err) will
not abort even if errp is set to error_abort.
Instead, we get an abort at error_propagate which is too late.
To fix, add an API to check errp and set local_err to error_abort
if errp is error_abort.
Michael S. Tsirkin (3):
error: don't rely on pointer comparisons
error: allow local errors to trigger abort
block/nfs: switch to error_init_local
include/qapi/error.h | 5 +++++
block/nfs.c | 2 +-
util/error.c | 21 ++++++++++++++++-----
3 files changed, 22 insertions(+), 6 deletions(-)
--
MST
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons
2015-06-16 12:53 [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort Michael S. Tsirkin
@ 2015-06-16 12:53 ` Michael S. Tsirkin
2015-06-16 14:45 ` Eric Blake
2015-06-16 15:03 ` Eric Blake
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort Michael S. Tsirkin
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-16 12:53 UTC (permalink / raw
To: qemu-devel; +Cc: kwolf, armbru, dgilbert
makes it possible to copy error_abort pointers,
not just pass them on directly.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
util/error.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/util/error.c b/util/error.c
index 14f4351..ccf29ea 100644
--- a/util/error.c
+++ b/util/error.c
@@ -20,7 +20,13 @@ struct Error
ErrorClass err_class;
};
-Error *error_abort;
+static Error error_abort_st = { .err_class = ERROR_CLASS_MAX };
+Error *error_abort = &error_abort_st;
+
+static bool error_is_abort(Error **errp)
+{
+ return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
+}
void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
{
@@ -40,7 +46,7 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
va_end(ap);
err->err_class = err_class;
- if (errp == &error_abort) {
+ if (error_is_abort(errp)) {
error_report_err(err);
abort();
}
@@ -76,7 +82,7 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
va_end(ap);
err->err_class = err_class;
- if (errp == &error_abort) {
+ if (error_is_abort(errp)) {
error_report_err(err);
abort();
}
@@ -121,7 +127,7 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
va_end(ap);
err->err_class = err_class;
- if (errp == &error_abort) {
+ if (error_is_abort(errp)) {
error_report_err(err);
abort();
}
@@ -168,7 +174,7 @@ void error_free(Error *err)
void error_propagate(Error **dst_errp, Error *local_err)
{
- if (local_err && dst_errp == &error_abort) {
+ if (local_err && error_is_abort(dst_errp)) {
error_report_err(local_err);
abort();
} else if (dst_errp && !*dst_errp) {
--
MST
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort
2015-06-16 12:53 [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort Michael S. Tsirkin
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons Michael S. Tsirkin
@ 2015-06-16 12:53 ` Michael S. Tsirkin
2015-06-16 15:06 ` Eric Blake
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local Michael S. Tsirkin
2015-06-17 14:26 ` [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort John Snow
3 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-16 12:53 UTC (permalink / raw
To: qemu-devel; +Cc: kwolf, armbru, dgilbert
It's a common idiom:
Error *local_err = NULL;
....
foo(&local_err);
...
if (local_err) {
error_propagate(errp, local_err);
return;
}
Unfortunately it means that call to foo(&local_err) will
not abort even if errp is set to error_abort.
Instead, we get an abort at error_propagate which is too late.
To fix, add an API to check errp and set local_err to error_abort
if errp is error_abort.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/qapi/error.h | 5 +++++
util/error.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index f44c451..8246a62 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -88,6 +88,11 @@ const char *error_get_pretty(Error *err);
void error_report_err(Error *);
/**
+ * Init a local error. It must be propagated to errp using error_propagate.
+ */
+Error *error_init_local(Error **errp);
+
+/**
* Propagate an error to an indirect pointer to an error. This function will
* always transfer ownership of the error reference and handles the case where
* dst_err is NULL correctly. Errors after the first are discarded.
diff --git a/util/error.c b/util/error.c
index ccf29ea..7489967 100644
--- a/util/error.c
+++ b/util/error.c
@@ -28,6 +28,11 @@ static bool error_is_abort(Error **errp)
return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
}
+Error *error_init_local(Error **errp)
+{
+ return error_is_abort(errp) ? *errp : NULL;
+}
+
void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
{
Error *err;
--
MST
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local
2015-06-16 12:53 [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort Michael S. Tsirkin
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons Michael S. Tsirkin
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort Michael S. Tsirkin
@ 2015-06-16 12:53 ` Michael S. Tsirkin
2015-06-16 15:08 ` Eric Blake
2015-06-17 14:26 ` [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort John Snow
3 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-16 12:53 UTC (permalink / raw
To: qemu-devel; +Cc: kwolf, qemu-block, Jeff Cody, Peter Lieven, armbru, dgilbert
We probably should just switch everyone, this is
just to demonstrate the API usage.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
block/nfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/nfs.c b/block/nfs.c
index ca9e24e..de4b8c3 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -385,7 +385,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
NFSClient *client = bs->opaque;
int64_t ret;
QemuOpts *opts;
- Error *local_err = NULL;
+ Error *local_err = error_init_local(errp);
client->aio_context = bdrv_get_aio_context(bs);
--
MST
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons Michael S. Tsirkin
@ 2015-06-16 14:45 ` Eric Blake
2015-06-16 14:49 ` Michael S. Tsirkin
2015-06-16 14:50 ` Eric Blake
2015-06-16 15:03 ` Eric Blake
1 sibling, 2 replies; 17+ messages in thread
From: Eric Blake @ 2015-06-16 14:45 UTC (permalink / raw
To: Michael S. Tsirkin, qemu-devel; +Cc: kwolf, armbru, dgilbert
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> makes it possible to copy error_abort pointers,
> not just pass them on directly.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> util/error.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
Where is this patch needed?
Is the goal to allow:
Error err = error_abort;
as a way to statically initialize err via copy to behave the same as the
global error_abort?
But that's not how you were using it in patch 3. There, you were
initializing Error *, so using &error_abort is still useful there, and
pointer equality still suffices.
I don't see a convincing use for this patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons
2015-06-16 14:45 ` Eric Blake
@ 2015-06-16 14:49 ` Michael S. Tsirkin
2015-06-16 14:50 ` Eric Blake
1 sibling, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-16 14:49 UTC (permalink / raw
To: Eric Blake; +Cc: kwolf, qemu-devel, dgilbert, armbru
On Tue, Jun 16, 2015 at 08:45:05AM -0600, Eric Blake wrote:
> On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> > makes it possible to copy error_abort pointers,
> > not just pass them on directly.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > util/error.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
>
> Where is this patch needed?
>
> Is the goal to allow:
>
> Error err = error_abort;
>
> as a way to statically initialize err via copy to behave the same as the
> global error_abort?
>
> But that's not how you were using it in patch 3. There, you were
> initializing Error *, so using &error_abort is still useful there, and
> pointer equality still suffices.
No because &error_abort is Error **.
> I don't see a convincing use for this patch.
See cover letter for the motivation.
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons
2015-06-16 14:45 ` Eric Blake
2015-06-16 14:49 ` Michael S. Tsirkin
@ 2015-06-16 14:50 ` Eric Blake
1 sibling, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-06-16 14:50 UTC (permalink / raw
To: Michael S. Tsirkin, qemu-devel; +Cc: kwolf, armbru, dgilbert
[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]
On 06/16/2015 08:45 AM, Eric Blake wrote:
> On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
>> makes it possible to copy error_abort pointers,
>> not just pass them on directly.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> util/error.c | 16 +++++++++++-----
>> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> Where is this patch needed?
>
> Is the goal to allow:
>
> Error err = error_abort;
Oh, my bad for not double checking the layers of indirection involved.
The above comment makes no sense type-wise, since error_abort is already
'Error *', and &error_abort is 'Error **'.
>
> as a way to statically initialize err via copy to behave the same as the
> global error_abort?
>
> But that's not how you were using it in patch 3. There, you were
> initializing Error *, so using &error_abort is still useful there, and
> pointer equality still suffices.
>
> I don't see a convincing use for this patch.
And I retract that, because now I do see the use.
Error *local_err = ...;
as a way to set an abort-on-error pointer requires that we have more
than just a global error_abort abort-on-error pointer, but that any
number of pointers all resolve to something with embedded properties.
Sorry for the confusion on my part, and:
Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons Michael S. Tsirkin
2015-06-16 14:45 ` Eric Blake
@ 2015-06-16 15:03 ` Eric Blake
2015-06-17 6:57 ` Michael S. Tsirkin
1 sibling, 1 reply; 17+ messages in thread
From: Eric Blake @ 2015-06-16 15:03 UTC (permalink / raw
To: Michael S. Tsirkin, qemu-devel; +Cc: kwolf, armbru, dgilbert
[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]
On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> makes it possible to copy error_abort pointers,
> not just pass them on directly.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> util/error.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/util/error.c b/util/error.c
> index 14f4351..ccf29ea 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -20,7 +20,13 @@ struct Error
> ErrorClass err_class;
> };
>
> -Error *error_abort;
> +static Error error_abort_st = { .err_class = ERROR_CLASS_MAX };
> +Error *error_abort = &error_abort_st;
Looking at this a bit further, I still wonder if we can do a slightly
better job of coming up with something that will SIGSEGV (or SIGBUS) if
we (accidentally) try to dereference the pointer (similar to how SIG_IGN
is (sighandler_t)1) - because we know that the abort object should never
be dereferenced. Something like:
Error *error_abort = (Error *)1;
with no need for error_abort_st. (Might have to spell it as Error
*error_abort = (void*)(intptr_t)1;
to shut up compiler warnings)
> +
> +static bool error_is_abort(Error **errp)
> +{
> + return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
> +}
and this would be:
return errp && *errp == error_abort;
The rest of this patch is still good. Then in patch 2, you'd have:
Error *error_init_local(Error **errp)
{
return error_is_abort(errp) ? error_abort : NULL;
}
That is, you still use pointer equality, but at one less level of
indirection (equality at the Error* level, not the Error** level).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort Michael S. Tsirkin
@ 2015-06-16 15:06 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-06-16 15:06 UTC (permalink / raw
To: Michael S. Tsirkin, qemu-devel; +Cc: kwolf, armbru, dgilbert
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> It's a common idiom:
>
> Error *local_err = NULL;
> ....
> foo(&local_err);
> ...
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> Unfortunately it means that call to foo(&local_err) will
> not abort even if errp is set to error_abort.
>
> Instead, we get an abort at error_propagate which is too late.
That is, the quality of the stack trace is degraded in that it no longer
pinpoints the actual cause of failure.
>
> To fix, add an API to check errp and set local_err to error_abort
> if errp is error_abort.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/qapi/error.h | 5 +++++
> util/error.c | 5 +++++
> 2 files changed, 10 insertions(+)
I like the idea.
> +++ b/util/error.c
> @@ -28,6 +28,11 @@ static bool error_is_abort(Error **errp)
> return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
> }
>
> +Error *error_init_local(Error **errp)
> +{
> + return error_is_abort(errp) ? *errp : NULL;
What you have works, but see also my ideas in my (latest) reply to 1/3
where you could use pointer equality on error_abort (rather than on
&error_abort) as the key factor. After all, the way you have
implemented things so far, when taking the *errp branch, you are still
effectively returning the error_abort global pointer (you haven't yet
used anything that required struct copying of the contents of error_abort).
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local Michael S. Tsirkin
@ 2015-06-16 15:08 ` Eric Blake
2015-06-16 21:17 ` Michael S. Tsirkin
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2015-06-16 15:08 UTC (permalink / raw
To: Michael S. Tsirkin, qemu-devel
Cc: kwolf, qemu-block, Jeff Cody, Peter Lieven, armbru, dgilbert
[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]
On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> We probably should just switch everyone, this is
> just to demonstrate the API usage.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> block/nfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
And indeed this is the reason things are still at RFC level. I like the
idea. It doesn't change anything for a bug-free program, but where we
DO have a bug, we now get a stacktrace that aborts as soon as possible
rather than delaying to the propagation point and losing some information.
>
> diff --git a/block/nfs.c b/block/nfs.c
> index ca9e24e..de4b8c3 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -385,7 +385,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
> NFSClient *client = bs->opaque;
> int64_t ret;
> QemuOpts *opts;
> - Error *local_err = NULL;
> + Error *local_err = error_init_local(errp);
Should be a fairly mechanical patch to catch all the spots; although
there are multiple spellings (not all callers name it local_err).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local
2015-06-16 15:08 ` Eric Blake
@ 2015-06-16 21:17 ` Michael S. Tsirkin
0 siblings, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-16 21:17 UTC (permalink / raw
To: Eric Blake
Cc: kwolf, qemu-block, Jeff Cody, Peter Lieven, qemu-devel, armbru,
dgilbert
On Tue, Jun 16, 2015 at 09:08:16AM -0600, Eric Blake wrote:
> On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> > We probably should just switch everyone, this is
> > just to demonstrate the API usage.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > block/nfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
>
> And indeed this is the reason things are still at RFC level. I like the
> idea. It doesn't change anything for a bug-free program, but where we
> DO have a bug, we now get a stacktrace that aborts as soon as possible
> rather than delaying to the propagation point and losing some information.
>
> >
> > diff --git a/block/nfs.c b/block/nfs.c
> > index ca9e24e..de4b8c3 100644
> > --- a/block/nfs.c
> > +++ b/block/nfs.c
> > @@ -385,7 +385,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags,
> > NFSClient *client = bs->opaque;
> > int64_t ret;
> > QemuOpts *opts;
> > - Error *local_err = NULL;
> > + Error *local_err = error_init_local(errp);
>
> Should be a fairly mechanical patch to catch all the spots; although
> there are multiple spellings (not all callers name it local_err).
I'll try to write an spatch to do this.
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons
2015-06-16 15:03 ` Eric Blake
@ 2015-06-17 6:57 ` Michael S. Tsirkin
2015-06-17 9:11 ` Kevin Wolf
0 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 6:57 UTC (permalink / raw
To: Eric Blake; +Cc: kwolf, qemu-devel, dgilbert, armbru
On Tue, Jun 16, 2015 at 09:03:44AM -0600, Eric Blake wrote:
> On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> > makes it possible to copy error_abort pointers,
> > not just pass them on directly.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > util/error.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/util/error.c b/util/error.c
> > index 14f4351..ccf29ea 100644
> > --- a/util/error.c
> > +++ b/util/error.c
> > @@ -20,7 +20,13 @@ struct Error
> > ErrorClass err_class;
> > };
> >
> > -Error *error_abort;
> > +static Error error_abort_st = { .err_class = ERROR_CLASS_MAX };
> > +Error *error_abort = &error_abort_st;
>
> Looking at this a bit further, I still wonder if we can do a slightly
> better job of coming up with something that will SIGSEGV (or SIGBUS) if
> we (accidentally) try to dereference the pointer (similar to how SIG_IGN
> is (sighandler_t)1) - because we know that the abort object should never
> be dereferenced. Something like:
>
> Error *error_abort = (Error *)1;
>
> with no need for error_abort_st. (Might have to spell it as Error
> *error_abort = (void*)(intptr_t)1;
> to shut up compiler warnings)
>
> > +
> > +static bool error_is_abort(Error **errp)
> > +{
> > + return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
> > +}
>
> and this would be:
>
> return errp && *errp == error_abort;
>
> The rest of this patch is still good. Then in patch 2, you'd have:
>
> Error *error_init_local(Error **errp)
> {
> return error_is_abort(errp) ? error_abort : NULL;
> }
>
> That is, you still use pointer equality, but at one less level of
> indirection (equality at the Error* level, not the Error** level).
It's a clever trick, it'd work. But why do tricks? This is never
performance-critical, is it? E.g. debugging is easier if pointers
actually point to things.
Let's see what do others say.
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons
2015-06-17 6:57 ` Michael S. Tsirkin
@ 2015-06-17 9:11 ` Kevin Wolf
0 siblings, 0 replies; 17+ messages in thread
From: Kevin Wolf @ 2015-06-17 9:11 UTC (permalink / raw
To: Michael S. Tsirkin; +Cc: qemu-devel, dgilbert, armbru
Am 17.06.2015 um 08:57 hat Michael S. Tsirkin geschrieben:
> On Tue, Jun 16, 2015 at 09:03:44AM -0600, Eric Blake wrote:
> > On 06/16/2015 06:53 AM, Michael S. Tsirkin wrote:
> > > makes it possible to copy error_abort pointers,
> > > not just pass them on directly.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > util/error.c | 16 +++++++++++-----
> > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/util/error.c b/util/error.c
> > > index 14f4351..ccf29ea 100644
> > > --- a/util/error.c
> > > +++ b/util/error.c
> > > @@ -20,7 +20,13 @@ struct Error
> > > ErrorClass err_class;
> > > };
> > >
> > > -Error *error_abort;
> > > +static Error error_abort_st = { .err_class = ERROR_CLASS_MAX };
> > > +Error *error_abort = &error_abort_st;
> >
> > Looking at this a bit further, I still wonder if we can do a slightly
> > better job of coming up with something that will SIGSEGV (or SIGBUS) if
> > we (accidentally) try to dereference the pointer (similar to how SIG_IGN
> > is (sighandler_t)1) - because we know that the abort object should never
> > be dereferenced. Something like:
> >
> > Error *error_abort = (Error *)1;
> >
> > with no need for error_abort_st. (Might have to spell it as Error
> > *error_abort = (void*)(intptr_t)1;
> > to shut up compiler warnings)
> >
> > > +
> > > +static bool error_is_abort(Error **errp)
> > > +{
> > > + return errp && *errp && (*errp)->err_class == ERROR_CLASS_MAX;
> > > +}
> >
> > and this would be:
> >
> > return errp && *errp == error_abort;
> >
> > The rest of this patch is still good. Then in patch 2, you'd have:
> >
> > Error *error_init_local(Error **errp)
> > {
> > return error_is_abort(errp) ? error_abort : NULL;
> > }
> >
> > That is, you still use pointer equality, but at one less level of
> > indirection (equality at the Error* level, not the Error** level).
>
> It's a clever trick, it'd work. But why do tricks? This is never
> performance-critical, is it? E.g. debugging is easier if pointers
> actually point to things.
>
> Let's see what do others say.
This isn't about performance, but about failing for invalid use. If we
say that dereferencing error_abort is wrong, then letting it fail fast
(with a segfault) actually makes debugging easier than silently
accessing garbage and trying to figure out why some code misbehaves only
later.
Kevin
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort
2015-06-16 12:53 [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort Michael S. Tsirkin
` (2 preceding siblings ...)
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local Michael S. Tsirkin
@ 2015-06-17 14:26 ` John Snow
2015-06-17 14:28 ` Michael S. Tsirkin
2015-06-17 14:29 ` Kevin Wolf
3 siblings, 2 replies; 17+ messages in thread
From: John Snow @ 2015-06-17 14:26 UTC (permalink / raw
To: Michael S. Tsirkin, qemu-devel; +Cc: kwolf, armbru, dgilbert
On 06/16/2015 08:53 AM, Michael S. Tsirkin wrote:
>
> It's a common idiom:
>
> Error *local_err = NULL;
> ....
> foo(&local_err);
> ...
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> Unfortunately it means that call to foo(&local_err) will
> not abort even if errp is set to error_abort.
>
> Instead, we get an abort at error_propagate which is too late.
>
Please humor the ignorant: Why is this too late? Any code that does
anything between foo(&local_err) and error_propagate is already broken.
> To fix, add an API to check errp and set local_err to error_abort
> if errp is error_abort.
>
> Michael S. Tsirkin (3):
> error: don't rely on pointer comparisons
> error: allow local errors to trigger abort
> block/nfs: switch to error_init_local
>
> include/qapi/error.h | 5 +++++
> block/nfs.c | 2 +-
> util/error.c | 21 ++++++++++++++++-----
> 3 files changed, 22 insertions(+), 6 deletions(-)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort
2015-06-17 14:26 ` [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort John Snow
@ 2015-06-17 14:28 ` Michael S. Tsirkin
2015-06-17 14:29 ` Kevin Wolf
1 sibling, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-17 14:28 UTC (permalink / raw
To: John Snow; +Cc: kwolf, qemu-devel, dgilbert, armbru
On Wed, Jun 17, 2015 at 10:26:26AM -0400, John Snow wrote:
>
>
> On 06/16/2015 08:53 AM, Michael S. Tsirkin wrote:
> >
> > It's a common idiom:
> >
> > Error *local_err = NULL;
> > ....
> > foo(&local_err);
> > ...
> > if (local_err) {
> > error_propagate(errp, local_err);
> > return;
> > }
> >
> > Unfortunately it means that call to foo(&local_err) will
> > not abort even if errp is set to error_abort.
> >
> > Instead, we get an abort at error_propagate which is too late.
> >
>
> Please humor the ignorant: Why is this too late? Any code that does
> anything between foo(&local_err) and error_propagate is already broken.
We have lost the stack so it's hard to debug. v2 explains it better.
> > To fix, add an API to check errp and set local_err to error_abort
> > if errp is error_abort.
> >
> > Michael S. Tsirkin (3):
> > error: don't rely on pointer comparisons
> > error: allow local errors to trigger abort
> > block/nfs: switch to error_init_local
> >
> > include/qapi/error.h | 5 +++++
> > block/nfs.c | 2 +-
> > util/error.c | 21 ++++++++++++++++-----
> > 3 files changed, 22 insertions(+), 6 deletions(-)
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort
2015-06-17 14:26 ` [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort John Snow
2015-06-17 14:28 ` Michael S. Tsirkin
@ 2015-06-17 14:29 ` Kevin Wolf
2015-06-17 14:30 ` John Snow
1 sibling, 1 reply; 17+ messages in thread
From: Kevin Wolf @ 2015-06-17 14:29 UTC (permalink / raw
To: John Snow; +Cc: armbru, qemu-devel, dgilbert, Michael S. Tsirkin
Am 17.06.2015 um 16:26 hat John Snow geschrieben:
>
>
> On 06/16/2015 08:53 AM, Michael S. Tsirkin wrote:
> >
> > It's a common idiom:
> >
> > Error *local_err = NULL;
> > ....
> > foo(&local_err);
> > ...
> > if (local_err) {
> > error_propagate(errp, local_err);
> > return;
> > }
> >
> > Unfortunately it means that call to foo(&local_err) will
> > not abort even if errp is set to error_abort.
> >
> > Instead, we get an abort at error_propagate which is too late.
> >
>
> Please humor the ignorant: Why is this too late? Any code that does
> anything between foo(&local_err) and error_propagate is already broken.
The interesting part is the stack trace which is truncated if you
abort() only in the outermost caller.
Kevin
> > To fix, add an API to check errp and set local_err to error_abort
> > if errp is error_abort.
> >
> > Michael S. Tsirkin (3):
> > error: don't rely on pointer comparisons
> > error: allow local errors to trigger abort
> > block/nfs: switch to error_init_local
> >
> > include/qapi/error.h | 5 +++++
> > block/nfs.c | 2 +-
> > util/error.c | 21 ++++++++++++++++-----
> > 3 files changed, 22 insertions(+), 6 deletions(-)
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort
2015-06-17 14:29 ` Kevin Wolf
@ 2015-06-17 14:30 ` John Snow
0 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-06-17 14:30 UTC (permalink / raw
To: Kevin Wolf; +Cc: armbru, qemu-devel, dgilbert, Michael S. Tsirkin
On 06/17/2015 10:29 AM, Kevin Wolf wrote:
> Am 17.06.2015 um 16:26 hat John Snow geschrieben:
>>
>>
>> On 06/16/2015 08:53 AM, Michael S. Tsirkin wrote:
>>>
>>> It's a common idiom:
>>>
>>> Error *local_err = NULL;
>>> ....
>>> foo(&local_err);
>>> ...
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>>
>>> Unfortunately it means that call to foo(&local_err) will
>>> not abort even if errp is set to error_abort.
>>>
>>> Instead, we get an abort at error_propagate which is too late.
>>>
>>
>> Please humor the ignorant: Why is this too late? Any code that does
>> anything between foo(&local_err) and error_propagate is already broken.
>
> The interesting part is the stack trace which is truncated if you
> abort() only in the outermost caller.
>
> Kevin
>
AHHH, that makes sense now, thank you.
>>> To fix, add an API to check errp and set local_err to error_abort
>>> if errp is error_abort.
>>>
>>> Michael S. Tsirkin (3):
>>> error: don't rely on pointer comparisons
>>> error: allow local errors to trigger abort
>>> block/nfs: switch to error_init_local
>>>
>>> include/qapi/error.h | 5 +++++
>>> block/nfs.c | 2 +-
>>> util/error.c | 21 ++++++++++++++++-----
>>> 3 files changed, 22 insertions(+), 6 deletions(-)
>>>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-06-17 14:31 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-16 12:53 [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort Michael S. Tsirkin
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons Michael S. Tsirkin
2015-06-16 14:45 ` Eric Blake
2015-06-16 14:49 ` Michael S. Tsirkin
2015-06-16 14:50 ` Eric Blake
2015-06-16 15:03 ` Eric Blake
2015-06-17 6:57 ` Michael S. Tsirkin
2015-06-17 9:11 ` Kevin Wolf
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort Michael S. Tsirkin
2015-06-16 15:06 ` Eric Blake
2015-06-16 12:53 ` [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local Michael S. Tsirkin
2015-06-16 15:08 ` Eric Blake
2015-06-16 21:17 ` Michael S. Tsirkin
2015-06-17 14:26 ` [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort John Snow
2015-06-17 14:28 ` Michael S. Tsirkin
2015-06-17 14:29 ` Kevin Wolf
2015-06-17 14:30 ` John Snow
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 read-only IMAP folder(s) and NNTP newsgroup(s).