apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r822263 - /apr/apr-util/branches/1.4.x/CHANGES
Date Wed, 07 Oct 2009 21:48:37 GMT
On Wed, Oct 7, 2009 at 5:11 PM, Graham Leggett <minfrin@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.

Mime
View raw message