| Message ID | 1434458200-23440-2-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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> >
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).
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 >
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
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) {
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(-)