Re: ldap unreleasable
On 3/28/2011 6:18 PM, Graham Leggett wrote: > > Looks like mod_ssl needs some similar cleanup, there are still style > violations in there, > not limited to the examples below. Now would be a good time to overhaul style. Agreed; it's much harder (and less justifiable) to make these sorts of changes mid-maintenance after a 2.4.0 tag, especially since they are harder to follow/ignore than whitespace changes. +1 to whomever wants to do the very significant work of un-Hungarianizing mod_ssl at this juncture. The reason I never did this is because SSL uses some Hungarian notation itself, and at the interface boundary it's less worrying. But you are pointing out httpd'isms and apr'isms which shouldn't have been expressed that way. FWIW I come from that background myself :) But my examples in httpd are limited to the places in mod_isapi where we intersect with Win32 constructs, so it actually makes the code more legible to know 'that data' is over on the isapi side of the world, and this other data is rooted in and managed by httpd.
Re: ldap unreleasable
On 29 Mar 2011, at 12:13 AM, William A. Rowe Jr. wrote:
/* LDAP cache state information */
typedef struct util_ldap_state_t {
...
int connectionPoolTTL;
} util_ldap_state_t;
I'm continue to grow more worried that the state of ldap in httpd
and in apr enjoys very little granularity, oversight, or quality...
1. Hungarian? Forgot to eat breakfast that day? Out of bounds
per httpd style rules.
2. int? Really? This is assigned an apr_interval_time_t in its
config code.
Please review and fix the style violations, and ensure that timeouts
are doing what they were meant to do.
Please be aware that the person who donated this code to us isn't an
ASF person, and donated the code in good faith. If it doesn't fit our
style guide, we should fix it, and show respect for their original
contribution.
Looks like mod_ssl needs some similar cleanup, there are still style
violations in there, not limited to the examples below. Now would be a
good time to overhaul style.
typedef struct {
...
} SSLConnRec;
typedef struct {
pid_t pid;
apr_pool_t *pPool;
BOOLbFixed;
apr_global_mutex_t *pMutex;
apr_array_header_t *aRandSeed;
apr_hash_t *tVHostKeys;
void *pTmpKeys[SSL_TMP_KEY_MAX];
apr_hash_t *tPublicCert;
apr_hash_t *tPrivateKey;
const char *szCryptoDevice;
struct {
void *pV1, *pV2, *pV3, *pV4, *pV5, *pV6, *pV7, *pV8, *pV9,
*pV10;
} rCtx;
} SSLModConfigRec;
Regards,
Graham
--
Re: ldap unreleasable
On Mon, Mar 28, 2011 at 6:13 PM, William A. Rowe Jr.
wrote:
> /* LDAP cache state information */
> typedef struct util_ldap_state_t {
> ...
> int connectionPoolTTL;
> } util_ldap_state_t;
>
>
> I'm continue to grow more worried that the state of ldap in httpd
> and in apr enjoys very little granularity, oversight, or quality...
>
> 1. Hungarian? Forgot to eat breakfast that day? Out of bounds
> per httpd style rules.
>
> 2. int? Really? This is assigned an apr_interval_time_t in its
> config code.
>
> Please review and fix the style violations, and ensure that timeouts
> are doing what they were meant to do.
>
Thanks for the review, blockers above in r1086432 (and bugfix in r1086433)
--
Eric Covener
cove...@gmail.com
ldap unreleasable
/* LDAP cache state information */
typedef struct util_ldap_state_t {
...
int connectionPoolTTL;
} util_ldap_state_t;
I'm continue to grow more worried that the state of ldap in httpd
and in apr enjoys very little granularity, oversight, or quality...
1. Hungarian? Forgot to eat breakfast that day? Out of bounds
per httpd style rules.
2. int? Really? This is assigned an apr_interval_time_t in its
config code.
Please review and fix the style violations, and ensure that timeouts
are doing what they were meant to do.
