Greg Stein wrote on Mon, Apr 02, 2012 at 05:51:19 -0400:
> On Sun, Apr 1, 2012 at 03:36, Daniel Shahaf <danie...@elego.de> wrote:
> > gst...@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
