Quanah Gibson-Mount wrote:
--On Friday, October 06, 2017 2:27 PM +0100 Howard Chu hyc@symas.com wrote:
Quanah Gibson-Mount wrote:
Suggested for RE24:
its7389 - Fix MozNSS to fallback to PEM if cert not found in certdb (RE24 ONLY) https://github.com/quanah/openldap-scratch/tree/its7389
Questionable, since the required PEM support module is 3rd party and not included in MozNSS. We have no way to test or support this patch.
This appears to be a fix for ITS#7276 (Added 2.4.32, 2012/07/31), which we already accepted into RE24. So it seems a legitimate fix to include in RE24.
OK.
its7442 - Add debug statements when index_intlen values are out of range https://github.com/quanah/openldap-scratch/tree/its7442
Looks pointless.
Well, the man page is not clear on this point. I'm fine dropping the debug statements, but what about the manpage updates which clarify the min/max allowed values?
Then we should also document all the other places where for example our integers accept a max value of 4294967295 or 18446744073709551615 too? It's stupid. Nobody is using 256 byte integers. Nobody is using integers bigger than 256 bytes. (Come on, 2^2048? really?) It's a limit that no one will ever hit in practice.
its8037 - Fix delta-syncrepl with relax https://github.com/quanah/openldap-scratch/tree/its8037
Looks like an enhancement, not a bugfix
I included this for RE24 as the reporter hit this problem with RE24. If we don't want to put it in RE24, are we OK for RE25/master?
Already approved it for RE25/master.
its8167 - Fix nonblocking TLS with referrals https://github.com/quanah/openldap-scratch/tree/its8167
OK, but non-blocking TLS was LDAP_DEVEL, not supported in RE24. This patch should be master/RE25 only.
I noted this for RE24 because the reporter was using the feature in RE24 (I.e., they specifically enabled it). Is there any harm in including (but not documenting via the changes file) it in RE24?
OK, leave it undoc'd in RE24.
its8605 - Fix various spelling errors https://github.com/quanah/openldap-scratch/tree/its8605
Introduces trailing whitespace - kill that before committing. In general, this patch falls under the "do not improve" rule http://www.openldap.org/devel/programming.html and should be rejected for not fixing any actual bugs. Many of the typos being fixed are in comments that are never user-visible anyway. Pollutes git history for a large number of files without any significant benefit.
Better leave it out of re24.
Is this ok for master/RE25 then?
It's still a bunch of changes that don't actually fix anything.
I guess we can take this in master. As a general rule, we should just reject patches like this in the future.
its8511 - Fix documentation for multimaster, deprecate mirrormode https://github.com/quanah/openldap-scratch/tree/its8511
Gratuitous change, existing docs and practices are already established. Hard enough to get people to update their docs, this is a bad idea.
This change is not gratuitous in the least. The misinformation in our current documentation leads to constant confusion among end users, who often do not want to go to the lengths necessary to deploy the *concept* that is mirror mode, and instead just want to do "multimaster", so they leave our current misnamed 'mirrormode' parameter set to false. Fixing the documentation to match the reality of what's being configured is a positive step to removing confusion and to stop misleading end users on what is being done. I've provided numerous links from the mailing list where this caused problems for end users before. Our parameters should reflect what they actually do.
You're talking about confusion for new users, meanwhile you're just creating confusion for existing users. Existing users tend to complain more because they have more invested into their running deployments. This is a bad idea.
its8573 - Add TLS options to ldap* tools https://github.com/quanah/openldap-scratch/tree/its8573-tables
The manpage updates are a bit excessive. Maybe we need a single manpage just for common options, that we can refer to from all of the individual commands' pages.
Ok, I'll add that to my RE25 stack of rework.
--Quanah
--
Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: http://www.symas.com
--On Friday, October 06, 2017 10:16 PM +0100 Howard Chu hyc@symas.com wrote:
its7442 - Add debug statements when index_intlen values are out of range https://github.com/quanah/openldap-scratch/tree/its7442
Looks pointless.
Well, the man page is not clear on this point. I'm fine dropping the debug statements, but what about the manpage updates which clarify the min/max allowed values?
Then we should also document all the other places where for example our integers accept a max value of 4294967295 or 18446744073709551615 too? It's stupid. Nobody is using 256 byte integers. Nobody is using integers bigger than 256 bytes. (Come on, 2^2048? really?) It's a limit that no one will ever hit in practice.
Well, I can certainly see why someone might expect they could set the minimum lower than 4 (even if that would be less than optimal). ;) I.e., you're focussed on maxsize, but the report covers both min & max. Any reason not to document that the default value of 4 is also the minimum allowed? I'm fine with dropping the patch as well.
its8511 - Fix documentation for multimaster, deprecate mirrormode https://github.com/quanah/openldap-scratch/tree/its8511
Gratuitous change, existing docs and practices are already established. Hard enough to get people to update their docs, this is a bad idea.
This change is not gratuitous in the least. The misinformation in our current documentation leads to constant confusion among end users, who often do not want to go to the lengths necessary to deploy the *concept* that is mirror mode, and instead just want to do "multimaster", so they leave our current misnamed 'mirrormode' parameter set to false. Fixing the documentation to match the reality of what's being configured is a positive step to removing confusion and to stop misleading end users on what is being done. I've provided numerous links from the mailing list where this caused problems for end users before. Our parameters should reflect what they actually do.
You're talking about confusion for new users, meanwhile you're just creating confusion for existing users. Existing users tend to complain more because they have more invested into their running deployments. This is a bad idea.
Since it deprecates the existing parameter, it has no effect on current deployments. If anyone ends up confused due to the fact that the documentation was clarified to reflect objective reality, I'll be more than happy to help them. We already have endless complaints that our documentation is overly confusing. This fix helps address that problem.
--Quanah
--
Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: http://www.symas.com
On 06/10/17 23:24, Quanah Gibson-Mount wrote:
--On Friday, October 06, 2017 10:16 PM +0100 Howard Chu hyc@symas.com wrote:
its7442 - Add debug statements when index_intlen values are out of range https://github.com/quanah/openldap-scratch/tree/its7442
Looks pointless.
Well, the man page is not clear on this point. I'm fine dropping the debug statements, but what about the manpage updates which clarify the min/max allowed values?
Then we should also document all the other places where for example our integers accept a max value of 4294967295 or 18446744073709551615 too? It's stupid. Nobody is using 256 byte integers. Nobody is using integers bigger than 256 bytes. (Come on, 2^2048? really?) It's a limit that no one will ever hit in practice.
Well, I can certainly see why someone might expect they could set the minimum lower than 4 (even if that would be less than optimal). ;) I.e., you're focussed on maxsize, but the report covers both min & max. Any reason not to document that the default value of 4 is also the minimum allowed? I'm fine with dropping the patch as well.
II like the original patch: Don't _silently_ change the user's config, report the changes.
Document "4 is default and minimum value".
The max - I suppose it fell out of the implementation somewhere. Don't lock us to that implementation. If it's even true - there's a char ibuf[64] in integerIndexer(), so maybe the 255 should be 64. Haven't looked closely. Anyway, remember keys of large integers get a floating format. Max integer _value_ is only limited by supported attribute value size.
-
Hallvard Breien Furuseth -
Howard Chu -
Quanah Gibson-Mount