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)
>> _("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" ?
Cheers,
-g
Received on 2012-04-02 11:51:51 CEST