[Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort

[Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort

[Michael S. Tsirkin]

    
    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

[Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons

[Michael S. Tsirkin]

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
> 

Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons

[Michael S. Tsirkin]

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
> 

Re: [Qemu-devel] [PATCH RFC 1/3] error: don't rely on pointer comparisons

[Kevin Wolf]

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

[Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RFC 2/3] error: allow local errors to trigger abort

[Eric Blake]

[-- 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 --]

[Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH RFC 3/3] block/nfs: switch to error_init_local

[Michael S. Tsirkin]

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
> 

Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort

[John Snow]

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(-)
> 

Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort

[Michael S. Tsirkin]

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(-)
> > 

Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort

[Kevin Wolf]

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(-)
> > 

Re: [Qemu-devel] [PATCH RFC 0/3] error: allow local errors to trigger abort

[John Snow]

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(-)
>>>