Re: [PATCH] trunk/2.4 core output filter is broken
On Saturday 04 February 2012, Gregg Smith wrote: This fixes the crash, thanks. Thanks. Committed to trunk and 2.4
Re: [PATCH] trunk/2.4 core output filter is broken
FanTAStic!!! On Feb 4, 2012, at 5:11 AM, Stefan Fritsch wrote: On Saturday 04 February 2012, Gregg Smith wrote: This fixes the crash, thanks. Thanks. Committed to trunk and 2.4
Re: [PATCH] trunk/2.4 core output filter is broken
On Thursday 02 February 2012, William A. Rowe Jr. wrote:
On 2/2/2012 8:36 AM, Jim Jagielski wrote:
bb == NULL ??
Looking at his attached screen scrape; no. Which leaves with
something like e == NULL or a broken bb.
The former :-(
Gregg, please add this or try the attached patch which is against
current trunk and includes all fixes so far. Thanks in advance.
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -743,11 +743,10 @@ apr_status_t
winnt_insert_network_bucket(conn_rec *c,
apr_bucket *e;
winnt_conn_ctx_t *context = ap_get_module_config(c-conn_config,
mpm_winnt_module);
-if (context == NULL)
+if (context == NULL || (e = context-overlapped.Pointer) == NULL)
return DECLINED;
/* seed the brigade with AcceptEx read heap bucket */
-e = context-overlapped.Pointer;
APR_BRIGADE_INSERT_HEAD(bb, e);
/* also seed the brigade with the client socket. */
e = apr_bucket_socket_create(socket, c-bucket_alloc);
diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index 3d6547d..faf24fe 100644
--- a/include/ap_mmn.h
+++ b/include/ap_mmn.h
@@ -390,12 +390,14 @@
* ap_core_ctx_get_bb(), move core_net rec definition
* to http_core.h
* 20120201.0 (2.5.0-dev) Bump MODULE_MAGIC_COOKIE to AP25!
+ * 20120203.0 (2.5.0-dev) Remove ap_create_core_ctx(), ap_core_ctx_get_bb();
+ * add insert_network_bucket hook
*/
#define MODULE_MAGIC_COOKIE 0x41503235UL /* AP25 */
#ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20120201
+#define MODULE_MAGIC_NUMBER_MAJOR 20120203
#endif
#define MODULE_MAGIC_NUMBER_MINOR 0 /* 0...n */
diff --git a/include/http_core.h b/include/http_core.h
index 9332e2e..1cfd182 100644
--- a/include/http_core.h
+++ b/include/http_core.h
@@ -704,24 +704,17 @@ typedef struct core_net_rec {
} core_net_rec;
/**
- * Allocate and fill the core_ctx_t for the core input filter, but don't
- * create a bucket with the input socket.
- * Normally this is done automatically when the core input filter is called
- * for the first time, but MPMs or protocol modules that need to do special
- * socket setup can call this function to do the initialization earlier.
- * They must add the input socket bucket to the core input filter's bucket
- * brigade, see ap_core_ctx_get_bb().
- * @param c The conn_rec of the connection
- * @return The core_ctx_t to be stored in core_net_rec-in_ctx
- */
-AP_DECLARE(core_ctx_t *) ap_create_core_ctx(conn_rec *c);
-
-/**
- * Accessor for the core input filter's bucket brigade
- * @param c The core_ctx_t to get the brigade from
- * @return The bucket brigade
- */
-AP_DECLARE(apr_bucket_brigade *) ap_core_ctx_get_bb(core_ctx_t *ctx);
+ * Insert the network bucket into the core input filter's input brigade.
+ * This hook is intended for MPMs or protocol modules that need to do special
+ * socket setup.
+ * @param c The connection
+ * @param bb The brigade to insert the bucket into
+ * @param socket The socket to put into a bucket
+ * @return DECLINED if the current function does not handle this connection,
+ * APR_SUCCESS or an error otherwise.
+ */
+AP_DECLARE_HOOK(apr_status_t, insert_network_bucket,
+(conn_rec *c, apr_bucket_brigade *bb, apr_socket_t *socket))
/* --
*
diff --git a/server/core.c b/server/core.c
index eb8147b..e05534b 100644
--- a/server/core.c
+++ b/server/core.c
@@ -82,12 +82,18 @@
APR_HOOK_STRUCT(
APR_HOOK_LINK(get_mgmt_items)
+APR_HOOK_LINK(insert_network_bucket)
)
AP_IMPLEMENT_HOOK_RUN_ALL(int, get_mgmt_items,
(apr_pool_t *p, const char *val, apr_hash_t *ht),
(p, val, ht), OK, DECLINED)
+AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket,
+(conn_rec *c, apr_bucket_brigade *bb,
+ apr_socket_t *socket),
+(c, bb, socket), DECLINED)
+
/* Server core module... This module provides support for really basic
* server operations, including options and commands which control the
* operation of other modules. Consider this the bureaucracy module.
@@ -4729,6 +4735,15 @@ AP_DECLARE(apr_uint32_t) ap_random_pick(apr_uint32_t min, apr_uint32_t max)
return number;
}
+static apr_status_t core_insert_network_bucket(conn_rec *c,
+ apr_bucket_brigade *bb,
+ apr_socket_t *socket)
+{
+apr_bucket *e = apr_bucket_socket_create(socket, c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(bb, e);
+return APR_SUCCESS;
+}
+
static void core_dump_config(apr_pool_t *p, server_rec *s)
{
core_server_config *sconf = ap_get_core_module_config(s-module_config);
@@ -4803,6 +4818,8 @@
Re: [PATCH] trunk/2.4 core output filter is broken
On Thursday 02 February 2012, Joe Orton wrote: The combination of APR_SUCCESS and DECLINED is unusual; an int return value with OK/DECLINED? Input and output filters should return an apr_status_t. So, if the hook does not return an apr_status_t, core_input_filter() would have to invent some apr_status_t value, which is bad. And I think the general principle that return code 'int' means HTTP_* error code should be kept. But I agree that DECLINED == -1 is not a good idea, because it could in theory collide with another APR_E* code. Should we define an AP_STATUS_DECLINED or something in the APR_OS_START_USERERR range? Or simply change the definition of DECLINED to be in the APR_OS_START_USERERR range? APR_OS_START_USERERR is 12, so there should be no danger of a collision with HTTP_*. It would require a major MMN bump, but making core_output_filter_ctx_t and core_ctx_t private is an API change, anyway.
Re: [PATCH] trunk/2.4 core output filter is broken
On 2/3/2012 10:43 AM, Stefan Fritsch wrote: On Thursday 02 February 2012, William A. Rowe Jr. wrote: On 2/2/2012 8:36 AM, Jim Jagielski wrote: bb == NULL ?? Looking at his attached screen scrape; no. Which leaves with something like e == NULL or a broken bb. The former :-( Gregg, please add this or try the attached patch which is against current trunk and includes all fixes so far. Thanks in advance. --- a/server/mpm/winnt/child.c +++ b/server/mpm/winnt/child.c @@ -743,11 +743,10 @@ apr_status_t winnt_insert_network_bucket(conn_rec *c, apr_bucket *e; winnt_conn_ctx_t *context = ap_get_module_config(c-conn_config, mpm_winnt_module); -if (context == NULL) +if (context == NULL || (e = context-overlapped.Pointer) == NULL) return DECLINED; /* seed the brigade with AcceptEx read heap bucket */ -e = context-overlapped.Pointer; APR_BRIGADE_INSERT_HEAD(bb, e); /* also seed the brigade with the client socket. */ e = apr_bucket_socket_create(socket, c-bucket_alloc); Stefan, This fixes the crash, thanks.
Re: [PATCH] trunk/2.4 core output filter is broken
bb == NULL ??
On Feb 2, 2012, at 3:40 AM, Gregg Smith wrote:
Stefan,
On 2/1/2012 12:44 PM, Stefan Fritsch wrote:
Try removing the static:
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -736,9 +736,9 @@ static winnt_conn_ctx_t
*winnt_get_connection(winnt_conn_ctx_t *context)
return context;
}
-static apr_status_t winnt_insert_network_bucket(conn_rec *c,
-apr_bucket_brigade
*bb,
-apr_socket_t *socket)
+apr_status_t winnt_insert_network_bucket(conn_rec *c,
+ apr_bucket_brigade *bb,
+ apr_socket_t *socket)
{
apr_bucket *e;
winnt_conn_ctx_t *context = ap_get_module_config(c-conn_config
That builds now, but one step forward, two steps back.
libhttpd crashes on any request with
AcceptFilter http none
or
AcceptFilter http connect
The default AcceptFilter http data works fine. I did not try https.
Regards,
Gregg
acfilter_crash.png
Re: [PATCH] trunk/2.4 core output filter is broken
On 2/2/2012 8:36 AM, Jim Jagielski wrote: bb == NULL ?? Looking at his attached screen scrape; no. Which leaves with something like e == NULL or a broken bb.
Re: [PATCH] trunk/2.4 core output filter is broken
On Sun, Jan 29, 2012 at 08:53:09PM +0100, Stefan Fritsch wrote: + * Insert the network bucket into the core input filter's input brigade. + * This hook is intended for MPMs or protocol modules that need to do special + * socket setup. + * @param c The connection + * @param bb The brigade to insert the bucket into + * @param socket The socket to put into a bucket + * @return DECLINED if the current function does not handle this connection, + * APR_SUCCESS or an error otherwise. + */ +AP_DECLARE_HOOK(apr_status_t, insert_network_bucket, +(conn_rec *c, apr_bucket_brigade *bb, apr_socket_t *socket)) The combination of APR_SUCCESS and DECLINED is unusual; an int return value with OK/DECLINED? Looks great otherwise - thanks Stefan!
Re: [PATCH] trunk/2.4 core output filter is broken
On 31.01.2012 22:30, Stefan Fritsch wrote: On Tuesday 31 January 2012, Gregg Smith wrote: On 1/29/2012 11:53 AM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. Patched trunk at r1237447, saw same after patching 2.4-HEAD so I tried trunk. Regards, Gregg Compiling... service.c .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module' : redefinition; different storage class c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70) : see declaration of 'mpm_winnt_module' Oops. Please try this in addition. Thanks for the testing. No info about Windows, but: I applied the patches on trunk and ran the test suite against a reallyall build on Solaris 10. No test failures. Regards, Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
On 1/31/2012 1:30 PM, Stefan Fritsch wrote: On Tuesday 31 January 2012, Gregg Smith wrote: On 1/29/2012 11:53 AM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. Patched trunk at r1237447, saw same after patching 2.4-HEAD so I tried trunk. Regards, Gregg Compiling... service.c .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module' : redefinition; different storage class c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70) : see declaration of 'mpm_winnt_module' Oops. Please try this in addition. Thanks for the testing. --- a/server/mpm/winnt/mpm_winnt.h +++ b/server/mpm/winnt/mpm_winnt.h @@ -67,7 +67,7 @@ void mpm_nt_eventlog_stderr_flush(void); /* From mpm_winnt.c: */ -extern module mpm_winnt_module; +extern module AP_MODULE_DECLARE_DATA mpm_winnt_module; extern int ap_threads_per_child; extern DWORD my_pid; Getting closer Linking... Creating library .\Release/libhttpd.lib and object .\Release/libhttpd.exp mpm_winnt.obj : error LNK2019: unresolved external symbol _winnt_insert_network_bucket referenced in function _winnt_hooks I don't quite get this error though.
Re: [PATCH] trunk/2.4 core output filter is broken
On Wednesday 01 February 2012, Gregg Smith wrote:
Getting closer
Linking...
Creating library .\Release/libhttpd.lib and object
.\Release/libhttpd.exp
mpm_winnt.obj : error LNK2019: unresolved external symbol
_winnt_insert_network_bucket referenced in function _winnt_hooks
I don't quite get this error though.
Having a Windows compiler myself would really help :-/
Try removing the static:
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -736,9 +736,9 @@ static winnt_conn_ctx_t
*winnt_get_connection(winnt_conn_ctx_t *context)
return context;
}
-static apr_status_t winnt_insert_network_bucket(conn_rec *c,
-apr_bucket_brigade
*bb,
-apr_socket_t *socket)
+apr_status_t winnt_insert_network_bucket(conn_rec *c,
+ apr_bucket_brigade *bb,
+ apr_socket_t *socket)
{
apr_bucket *e;
winnt_conn_ctx_t *context = ap_get_module_config(c-conn_config,
Re: [PATCH] trunk/2.4 core output filter is broken
On 1/29/2012 11:53 AM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. Patched trunk at r1237447, saw same after patching 2.4-HEAD so I tried trunk. Regards, Gregg Compiling... service.c .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module' : redefinition; different storage class c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70) : see declaration of 'mpm_winnt_module' scoreboard.c provider.c nt_eventlog.c mpm_winnt.c .\server\mpm\winnt\mpm_winnt.c(1765) : error C2370: 'mpm_winnt_module' : redefinition; different storage class c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70) : see declaration of 'mpm_winnt_module' .\server\mpm\winnt\mpm_winnt.c(1765) : error C2370: 'mpm_winnt_module' : redefinition; different storage class c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70) : see declaration of 'mpm_winnt_module' mpm_common.c listen.c child.c .\server\mpm\winnt\child.c(69) : error C2370: 'mpm_winnt_module' : redefinition; different storage class c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70) : see declaration of 'mpm_winnt_module'
Re: [PATCH] trunk/2.4 core output filter is broken
Hi Stefan, On 29.01.2012 20:53, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. as far as I understand, this patch is for trunk. Testing against 2.4.x would in addition need to first apply the following four patches (all the trunk commits apply clean to 2.4.x): r1236122 | rpluem | 2012-01-26 11:03:36 +0100 (Thu, 26 Jan 2012) | 1 line * Don't typedef twice (in .c and .h file). r1233882 | jorton | 2012-01-20 13:41:18 +0100 (Fri, 20 Jan 2012) | 4 lines * server/core_filters.c (ap_core_input_filter): Only treat EAGAIN as success if a non-blocking read was requested; for a blocking read, it is an error condition. r1234899 | jorton | 2012-01-23 17:57:07 +0100 (Mon, 23 Jan 2012) | 4 lines * server/core_filters.c (send_brigade_nonblocking): Use a non-blocking bucket read, allowing any pending data to be flushed before trying a (potentially slow) blocking read. r1235019 | sf | 2012-01-23 22:58:42 +0100 (Mon, 23 Jan 2012) | 6 lines Make the core input/output filter contexts private and provide accessor APIs for mpm_winnt and mod_ftp. This allows to add members to the context structs without breaking binary compatibility. Is that your understanding as well? Or are some of those unwanted for 2.4.x? Regards, Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
On Tuesday 31 January 2012, Gregg Smith wrote: On 1/29/2012 11:53 AM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. Patched trunk at r1237447, saw same after patching 2.4-HEAD so I tried trunk. Regards, Gregg Compiling... service.c .\server\mpm\winnt\service.c(38) : error C2370: 'mpm_winnt_module' : redefinition; different storage class c:\build3\httpd-head_r1237447\server\mpm\winnt\mpm_winnt.h(70) : see declaration of 'mpm_winnt_module' Oops. Please try this in addition. Thanks for the testing. --- a/server/mpm/winnt/mpm_winnt.h +++ b/server/mpm/winnt/mpm_winnt.h @@ -67,7 +67,7 @@ void mpm_nt_eventlog_stderr_flush(void); /* From mpm_winnt.c: */ -extern module mpm_winnt_module; +extern module AP_MODULE_DECLARE_DATA mpm_winnt_module; extern int ap_threads_per_child; extern DWORD my_pid;
Re: [PATCH] trunk/2.4 core output filter is broken
Anyone with Windows willing to sign up to review/test? On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. insert_network_bucket.diff
Re: [PATCH] trunk/2.4 core output filter is broken
On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. insert_network_bucket.diff Just a quick scan but: +AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket, +(conn_rec *c, apr_bucket_brigade *bb, + apr_socket_t *socket), +(c, bb, socket), DECLINED) + ... +ap_hook_insert_network_bucket(core_insert_network_bucket, NULL, NULL, + APR_HOOK_REALLY_LAST); looks wonky...
Re: [PATCH] trunk/2.4 core output filter is broken
On Mon, 30 Jan 2012, Jim Jagielski wrote: On Jan 29, 2012, at 2:53 PM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. insert_network_bucket.diff Just a quick scan but: +AP_IMPLEMENT_HOOK_RUN_FIRST(apr_status_t, insert_network_bucket, +(conn_rec *c, apr_bucket_brigade *bb, + apr_socket_t *socket), +(c, bb, socket), DECLINED) + ... +ap_hook_insert_network_bucket(core_insert_network_bucket, NULL, NULL, + APR_HOOK_REALLY_LAST); looks wonky... I fail to see what is wonky here. Can you be more specific?
Re: [PATCH] trunk/2.4 core output filter is broken
On 1/30/2012 7:51 AM, Jim Jagielski wrote: Anyone with Windows willing to sign up to review/test? I don't have a build environment to create something based on the diff, but if someone can create a build , I'll happily do the testing. -- Daniel Ruggeri
Re: [PATCH] trunk/2.4 core output filter is broken
On 31.01.2012 00:36, Daniel Ruggeri wrote: On 1/30/2012 7:51 AM, Jim Jagielski wrote: Anyone with Windows willing to sign up to review/test? I don't have a build environment to create something based on the diff, but if someone can create a build , I'll happily do the testing. What's you runtime env then (Win version and 32 or 64 Bits)? Rainer
Re: [PATCH] trunk/2.4 core output filter is broken
On 1/31/2012 12:01 AM, Rainer Jung wrote: What's you runtime env then (Win version and 32 or 64 Bits)? Rainer I have at my immediate disposal Server 2003 32-bit as well as Win7 and Server 2008 64-bit. I may be able to scrounge up some more exotic configurations if needed. My earlier tests to duplicate the problem were done on the Win7 64 box. -- Daniel Ruggeri
Re: [PATCH] trunk/2.4 core output filter is broken
On 1/29/2012 11:53 AM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. I'm assuming this is part of what is needed to get 2.4 going as well as Joe's big core overhaul in r1235019? I'm willing yet would prefer patching 2.4 for testing than patch build trunk.
Re: [PATCH] trunk/2.4 core output filter is broken
On 1/30/2012 11:14 PM, Gregg Smith wrote: On 1/29/2012 11:53 AM, Stefan Fritsch wrote: On Wednesday 25 January 2012, Jim Jagielski wrote: Looking over the code, impl as a hook seems more isolated, rather than the current impl which is intrusive (which is part of what we're trying to avoid, aren't we?) OK, patch is attached. This needs review/testing for Windows. I'm assuming this is part of what is needed to get 2.4 going as well as Joe's big core overhaul in r1235019? s/Joe's/Stefan's/ I'm willing yet would prefer patching 2.4 for testing than patch build trunk.
