You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2018/05/11 15:34:49 UTC
Re: svn commit: r1831396 -
/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> On May 11, 2018, at 5:44 AM, ylavic@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;
???
Re: svn commit: r1831396 - /httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
Posted by Yann Ylavic <yl...@gmail.com>.
BTW, an even simpler patch could be:
Index: modules/proxy/mod_proxy_balancer.c
===================================================================
--- modules/proxy/mod_proxy_balancer.c (revision 1831396)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -1886,25 +1886,12 @@ static void balancer_child_init(apr_pool_t *p, ser
apr_status_t rv;
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
- * 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.
+ /* The slotmem can't be NULL here with any MPM, initialized in
+ * post_config() it's either fork()ed on unixes MPMs, or directly
+ * available for fully threaded MPMs and winnt where post_config()
+ * is called in children processes too.
*/
- rv = storage->attach(&conf->bslot, conf->id, &size, &num, p);
- if (!conf->bslot) {
- ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s,
APLOGNO(01205) "slotmem_attach failed");
- exit(1); /* Ugly, but what else? */
- }
- (void)rv;
+ ap_assert(conf->bslot != NULL);
}
balancer = (proxy_balancer *)conf->balancers->elts;
_
since mod_proxy_balancer uses slotmem_shm only, attach() is not needed.
Re: svn commit: r1831396 - /httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, May 11, 2018 at 5:34 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>
>> On May 11, 2018, at 5:44 AM, ylavic@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?