On Fri, May 11, 2018 at 5:34 PM, Jim Jagielski <j...@jagunet.com> wrote:
>
>
>> On May 11, 2018, at 5:44 AM, yla...@apache.org 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?

Reply via email to