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

[Kevin Wolf <kwolf@redhat.com>]

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