On Monday 23 January 2012, Joe Orton wrote:
> > > Good catch on ctx->bytes_in. I'd add: why is
> > > core_output_filter_ctx_t in a public header?
> >
> >
> >
> > There is no good reason other than that other core filter structs
> > like core_filter_ctx and core_net_rec are exposed, too. And
> > those are actually used by the winnt MPM. I would prefer if all
> > these structs were private and core_filters.c would provide an
> > API to allocate them from the winnt MPM and add additional
> > buckets to the input brigade.
>
> Yes! I totally agree.
>
> > Is this something we should still do for 2.4.x (iff 2.4.0 is not
> > released)?
Attached is a simple patch to hide core input/output filter contexts.
The core_net_rec stays public. Obviously the winnt part is not tested
at all. The mod_ftp part compiles (after changing c->remote_ip
everywhere), docs are still missing.
This patch allows us to later add members to core_ctx_t without
breaking binary compatibility to mod_ftp. Without such a patch, the
size of core_ctx_t is part of the ABI, which is bad.
Opinions?
Index: modules/ftp/ftp_data_connection.c
===================================================================
--- modules/ftp/ftp_data_connection.c (Revision 1234692)
+++ modules/ftp/ftp_data_connection.c (Arbeitskopie)
@@ -280,18 +282,15 @@
for (f = cdata->input_filters; f; f = f->next) {
if (strcasecmp(f->frec->name, "CORE_IN") == 0) {
core_net_rec *net = f->ctx;
+ apr_bucket_brigade *bb;
apr_bucket *e;
- net->in_ctx = apr_pcalloc(fc->data_pool, sizeof(*net->in_ctx));
- net->in_ctx->b = apr_brigade_create(fc->data_pool,
- f->c->bucket_alloc);
- net->in_ctx->tmpbb =
- apr_brigade_create(net->in_ctx->b->p,
- net->in_ctx->b->bucket_alloc);
+ net->in_ctx = ap_create_core_ctx(f->c);
+ bb = ap_core_ctx_get_bb(net->in_ctx);
/* seed the brigade with our client data+control sockets */
e = ftp_bucket_datasock_create(fc, f->c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(net->in_ctx->b, e);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
break;
}
}
diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index 2f9e6bb..e77d655 100644
--- a/include/ap_mmn.h
+++ b/include/ap_mmn.h
@@ -385,12 +385,15 @@
* 20111203.1 (2.5.0-dev) Add ap_list_provider_groups()
* 20120109.0 (2.5.0-dev) Changes sizeof(overrides_t) in core config.
* 20120111.0 (2.5.0-dev) Remove sb_type from global_score.
+ * 20120123.0 (2.5.0-dev) Make core_output_filter_ctx_t and core_ctx_t
+ * private, add ap_create_core_ctx(),
+ * ap_core_ctx_get_bb()
*/
#define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
#ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20120111
+#define MODULE_MAGIC_NUMBER_MAJOR 20120123
#endif
#define MODULE_MAGIC_NUMBER_MINOR 0 /* 0...n */
diff --git a/include/httpd.h b/include/httpd.h
index 90bbc10..0e5cf35 100644
--- a/include/httpd.h
+++ b/include/httpd.h
@@ -1270,18 +1270,8 @@ struct server_rec {
void *context;
};
-typedef struct core_output_filter_ctx {
- apr_bucket_brigade *buffered_bb;
- apr_bucket_brigade *tmp_flush_bb;
- apr_pool_t *deferred_write_pool;
- apr_size_t bytes_in;
- apr_size_t bytes_written;
-} core_output_filter_ctx_t;
-
-typedef struct core_filter_ctx {
- apr_bucket_brigade *b;
- apr_bucket_brigade *tmpbb;
-} core_ctx_t;
+typedef struct core_output_filter_ctx core_output_filter_ctx_t;
+typedef struct core_filter_ctx core_ctx_t;
typedef struct core_net_rec {
/** Connection to the client */
@@ -1294,6 +1284,9 @@ typedef struct core_net_rec {
core_ctx_t *in_ctx;
} core_net_rec;
+AP_DECLARE(core_ctx_t *) ap_create_core_ctx(conn_rec *c);
+AP_DECLARE(apr_bucket_brigade *) ap_core_ctx_get_bb(core_ctx_t *ctx);
+
/**
* Get the context_document_root for a request. This is a generalization of
* the document root, which is too limited in the presence of mappers like
diff --git a/server/core_filters.c b/server/core_filters.c
index 42da5c2..275ff42 100644
--- a/server/core_filters.c
+++ b/server/core_filters.c
@@ -78,6 +78,32 @@ do { \
#undef APLOG_MODULE_INDEX
#define APLOG_MODULE_INDEX AP_CORE_MODULE_INDEX
+typedef struct core_output_filter_ctx {
+ apr_bucket_brigade *buffered_bb;
+ apr_bucket_brigade *tmp_flush_bb;
+ apr_pool_t *deferred_write_pool;
+ apr_size_t bytes_written;
+} core_output_filter_ctx_t;
+
+typedef struct core_filter_ctx {
+ apr_bucket_brigade *b;
+ apr_bucket_brigade *tmpbb;
+} core_ctx_t;
+
+
+AP_DECLARE(core_ctx_t *) ap_create_core_ctx(conn_rec *c)
+{
+ core_ctx_t *ctx = apr_palloc(c->pool, sizeof(*ctx));
+ ctx->b = apr_brigade_create(c->pool, c->bucket_alloc);
+ ctx->tmpbb = apr_brigade_create(c->pool, c->bucket_alloc);
+ return ctx;
+}
+
+AP_DECLARE(apr_bucket_brigade *) ap_core_ctx_get_bb(core_ctx_t *ctx)
+{
+ return ctx->b;
+}
+
int ap_core_input_filter(ap_filter_t *f, apr_bucket_brigade *b,
ap_input_mode_t mode, apr_read_type_e block,
apr_off_t readbytes)
@@ -105,18 +131,10 @@ int ap_core_input_filter(ap_filter_t *f, apr_bucket_brigade *b,
if (!ctx)
{
- /*
- * Note that this code is never executed on Windows because the winnt
- * MPM does the setup of net->in_ctx.
- * XXX: This should be fixed.
- */
- ctx = apr_pcalloc(f->c->pool, sizeof(*ctx));
- ctx->b = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
- ctx->tmpbb = apr_brigade_create(ctx->b->p, ctx->b->bucket_alloc);
+ net->in_ctx = ctx = ap_create_core_ctx(f->c);
/* seed the brigade with the client socket. */
e = apr_bucket_socket_create(net->client_socket, f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(ctx->b, e);
- net->in_ctx = ctx;
}
else if (APR_BRIGADE_EMPTY(ctx->b)) {
return APR_EOF;
diff --git a/server/mpm/winnt/child.c b/server/mpm/winnt/child.c
index ab4fddc..30a525a 100644
--- a/server/mpm/winnt/child.c
+++ b/server/mpm/winnt/child.c
@@ -812,7 +812,6 @@ static DWORD __stdcall worker_main(void *thread_num_val)
}
else if (e)
{
- core_ctx_t *ctx;
core_net_rec *net;
ap_filter_t *filt;
@@ -820,24 +819,20 @@ static DWORD __stdcall worker_main(void *thread_num_val)
while ((strcmp(filt->frec->name, "core_in") != 0) && filt->next)
filt = filt->next;
net = filt->ctx;
- ctx = net->in_ctx;
- if (net->in_ctx)
- ctx = net->in_ctx;
- else
+ if (net->in_ctx == NULL)
{
- ctx = apr_pcalloc(c->pool, sizeof(*ctx));
- ctx->b = apr_brigade_create(c->pool, c->bucket_alloc);
- ctx->tmpbb = apr_brigade_create(c->pool, c->bucket_alloc);
+ core_ctx_t *ctx = ap_create_core_ctx(c);
+ apr_bucket_brigade *bb = ap_core_ctx_get_bb(ctx);
/* seed the brigade with AcceptEx read heap bucket */
e = context->overlapped.Pointer;
- APR_BRIGADE_INSERT_HEAD(ctx->b, e);
+ APR_BRIGADE_INSERT_HEAD(bb, e);
/* also seed the brigade with the client socket. */
e = apr_bucket_socket_create(net->client_socket,
c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(ctx->b, e);
+ APR_BRIGADE_INSERT_TAIL(bb, e);
net->in_ctx = ctx;
}
}
