[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1307923 - /subversion/trunk/subversion/libsvn_subr/crypto.c

From: Daniel Shahaf <danielsh_at_elego.de>
Date: Mon, 2 Apr 2012 16:42:10 +0300

Greg Stein wrote on Mon, Apr 02, 2012 at 05:51:19 -0400:
> On Sun, Apr 1, 2012 at 03:36, Daniel Shahaf <danielsh_at_elego.de> wrote:
> > gstein_at_apache.org wrote on Sat, Mar 31, 2012 at 22:24:00 -0000:
> >> @@ -137,9 +159,15 @@ svn_crypto__context_create(svn_crypto__c
> >>
> >>    apr_err = apr_crypto_get_driver(&driver, "openssl", NULL, &apu_err,
> >>                                    result_pool);
> >> -  if (apr_err != APR_SUCCESS || driver == NULL)
> >> +  /* Potential bugs in get_driver() imply we might get APR_SUCCESS and NULL.
> >> +     Sigh. Just be a little more careful in error generation here.  */
> >> +  if (apr_err != APR_SUCCESS)
> >>      return svn_error_create(apr_err, err_from_apu_err(apr_err, apu_err),
> >
> > The code still dereferences APU_ERR when APR_ERR != APR_SUCCESS..
>
> That's fine. The first thing get_driver() does is to set apu_err to
> NULL. So it will be NULL or valid.
>
> Yes, this is an implementation detail, but apr(util) doesn't have the
> same sorts of API specification like we have (ie. if an error is
> returned, don't count on any OUT params). Without those details, we
> gotta look at the code. (sigh)
>

OK, thanks.

> >>                              _("OpenSSL crypto driver error"));
> >> +  if (driver == NULL)
> >> +    return svn_error_create(APR_EGENERAL,
> >> +                            err_from_apu_err(APR_EGENERAL, apu_err),
> >> +                            _("Bad return value while loading"));
> >
> > The error message doesn't say much when seen without context.  (Consider
> > the SASL error messages that we had to s/%s/SASL said: %s/.)  IMO it
> > should say something about "Creating crypto context" or such.
>
> Yeah. I made it a bit lame on purpose because I wasn't so sure what to
> tell users, and for the translators. I figured as long as it was
> unique, we would know what was going on, even if the user didn't. How
> about "Bad return value while loading crypto driver" ?
>

+1.

May want even to say "openssl" in the error, too --- the name of the
driver we load --- it'll probably save me telling some people "That
error probably means you have multiple OpenSSL .so's" on users@ :)

> Cheers,
> -g
Received on 2012-04-02 15:42:47 CEST

This is an archived mail posted to the Subversion Dev mailing list.