On Wed, Oct 7, 2009 at 5:11 PM, Graham Leggett <minf...@sharp.fm> wrote: > Jeff Trawick wrote: > >> I believe the key differences are in there. I'll add back the "disabled >> by default" aspect. First, >> please call out explicitly the other CHANGES entries I removed for which >> you would like an explanation. > > wrowe has made a number of calls for people to review the crypto code in > APR v1.4, and the entries in CHANGES are a key part of such a review. > > As I requested in my previous mail, please revert (as in, -1).
No, not at this time. I don't think your reasoning for the veto applies to these changes. (I don't think CHANGES entries are a key part of any new feature review. Users just have to know the feature exists. Presumably programmers will then look through the include files for keywords and find the documentation.) I think it is worth mentioning that I posted the patch four days before I committed, and two of our colleagues approved it. That doesn't mean it can't be changed, or even veto-ed, but perhaps with that in mind you'd accept that it is fair to call out specific entries within the commit that you disagree with for the reason you stated. Here they are one by one: *) Fix the saving of the old LIBS, CPPFLAGS and LDFLAGS when OpenSSL - and NSS are detected. [Graham Leggett, Ruediger Pluem] - - *) apr_crypto_nss: Oh that it was this easy. Use pkg-config to find - NSS where possible. [Graham Leggett] - - *) apr_crypto_nss: The nss.h header file could be in nss or nss3, the - prerror.h header file could be in nspr4. [Graham Leggett] - *) Fix a bogus initialisation of the IV size in the NSS crypto driver. - [Graham Leggett] - - *) Make sure that the underlying result code during driver initialisation - is exposed to the caller. [Graham Leggett] - - *) Provide a mechanism to provide the recommended crypto driver to - calling application. [Graham Leggett] - - *) Move APU_HAVE_CRYPTO from private apu_config.h to public apu.h. - [Ruediger Pluem, Graham Leggett] My understanding: Each of these deleted entries thus far is a minor implementation detail related to the new apr_crypto feature. As the feature was never released, they do not fix a problem for the users of our library. OTOH, any of these if tied to a particular user symptom could be interesting in the change log for a bug fix release, such as 1.4.1. Why is this understanding is wrong, or at least why is this information needed by reviewers of the apr_crypto feature? (We can find those reviewers a better place to find this kind of information, such as a handful of URLs taking them to the the commit logs.) <I've omitted the combination of the apr_crypto new-feature entries, which we discussed earlier and which I'm already said I'd change once I understand what to do with the rest.> - - *) Expose the apr_dso_handle_t when calling apu_dso_load, so that the - crypto code can call apr_dso_error and find out why the dso load - failed. The existing LDAP and DBD code ignores this, as their APIs - do not yet allow for errors to be returned. [Graham Leggett] My understanding: Internal implementation detail that does not fix any released code. Released or not, by itself it isn't interesting for CHANGES. (If APR-util 1.4.x had a new apr_dbd_foo_ex() API that has a new parameter to be filled in with a low-level load failure, then that new API would be logged in CHANGES; but this particular aspect isn't interesting.) I don't see why any of the apr_crypto reviewers need to see this.
