On Fri, May 11, 2018 at 5:34 PM, Jim Jagielski <[email protected]> wrote:
>
>
>> On May 11, 2018, at 5:44 AM, [email protected] wrote:
>>
>> + rv = storage->attach(&conf->bslot, conf->id, &size, &num, p);
>> if (!conf->bslot) {
>> - ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(01205)
>> "slotmem_attach failed");
>> + ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s, APLOGNO(01205)
>> "slotmem_attach failed");
>> exit(1); /* Ugly, but what else? */
>> }
>> + (void)rv;
>
> ???
Well, without the above comment I agree that it's not really
meaningful (it shouldn't hurt though).
What I meant is that the ->attach() API (put on the callee) is not
really friendly: don't touch &bslot unless it's a real error where you
should set it to NULL.
So what the comment proposes is something like this (in trunk only):
Index: modules/proxy/mod_proxy_balancer.c
===================================================================
--- modules/proxy/mod_proxy_balancer.c (revision 1831396)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -1888,23 +1888,23 @@ static void balancer_child_init(apr_pool_t *p, ser
if (conf->balancers->nelts) {
apr_size_t size;
unsigned int num;
- /* In 2.4.x we rely on the provider to return either the same
+ ap_slotmem_instance_t *bslot = conf->bslot;
+ /* In 2.4.x we relied on the provider to return either the same
* in/out &bslot, a valid new one, or NULL for failure/exit().
- * TODO? for 2.6+/3.x we possibly could consider returned status
- * to be real failures, but e.g. NOTFOUND/ENOSHM* to continue with
- * existing conf->bslot (even when the returned one is NULL).
- * Hence handle the slotmem reuse it here where we know it's valid
- * both for fork()ed post_config()s and MPM winnt-like ones (run in
- * child process too). The provider tells what it attached or not,
- * and if not whether the child should stop (fatal) or continue
- * with the "inherited" configuration.
+ * We now consider &bslot as out only, we know here that the one
+ * in conf->bslot can be reused if the provider doesn't find a SHM,
+ * any other error is a failure. This change can't be backported to
+ * 2.4.x, though.
*/
- rv = storage->attach(&conf->bslot, conf->id, &size, &num, p);
- if (!conf->bslot) {
+ rv = storage->attach(&bslot, conf->id, &size, &num, p);
+ if (rv == APR_SUCCESS) {
+ conf->bslot = bslot;
+ }
+ else if (!conf->bslot || (rv != APR_NOTFOUND &&
+ rv != APR_ENOSHMAVAIL)) {
ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s,
APLOGNO(01205) "slotmem_attach failed");
exit(1); /* Ugly, but what else? */
}
- (void)rv;
}
Which makes the culprit line vanish, but mainly avoids the weird
in/out semantics on the slotmem.
Sounds better?
