httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From TOKI...@aol.com
Subject Re: [PATCH] bucket problems.
Date Wed, 17 Jan 2001 11:07:05 GMT
In a message dated 01-01-17 09:33:53 EST, Ryan writes...

>  This patch addresses Dean's problems with the bucket code. 
...
>  Please review ASAP, I would like to commit VERY quickly.

Yikes.

If you look at what your patch actually does
you will see that it's really very simple... it's
substituting an 'e' brigade pointer for the
'r' request pointer in the 'ap_XXXX()' calls
and it adds a few 'bucket init' calls to the
module to get this 'default brigade' set up.

Am I missing something or is this like API
programming 101 and the total rewrite of all
the ap_rXXXX() calls is not necessary?

It is the ap_rXXXX() API calls themselves that can
simply 'know what to do' so legacy modules don't have
to suffer this kind of total rewrite.

Couldn't you just keep mod_autoindex exactly the way
it is and simply do this...

1. Add a 'default brigade' pointer to the request
record itself. Make sure it's NULL on request init.
Might get used, might not.

2. When a module (legacy) that ONLY uses ap_rXXXX() calls 
starts sending data the ap_rXXXX() calls simply check to see
whether the 'default' brigade has been initialized
and, if not, they go ahead auto-init r->default_brigade.
The ap_rXXXX() call then adds the output data to the
brigade normally and the caller isn't really aware
that a 'default' brigade is now in use and there is
no need for the caller to know about it. It's a lower-level
thing and should be handled automatically.

The only reason you are changing various module function
names as well is just so you can get this 'brigade'
pointer passed around. If a r->default_brigade become
the operative brigade then none of these changes
are needed either since 'r' is already being passed
around by all the functions you are changing.

Heck... even if the module still needed to do the
brigade init itself then simply being able to STORE
it in 'r->default_brigade' would prevent having to
change all the module function declarations. Any
existing module routine could just access the
default brigade via 'r->default_brigade'.

Every single ap_rXXXX() call already gets the 'r->'
request record passed so why can't that be the key to
having them KNOW the 'r->default_brigade' value as
well and avoid all these rewrites?

* PATCH SUMMARY

Below is a SUMMARY (only) of the patch you submitted.
It contains every single MINUS (-) and PLUS (+) line
from the patch excluding the files that are not
module specific and contain lower level bucket calls.

ONLY the MINUS (-) and PLUS (+) lines from the patch
are included and the rest of the 'noise' is eliminated
since I am simply making a point that only requires you
to look at the ACTUAL changes being made by the patch.

If you look at each and every change ( as I did ) you
will see that all that is really happening here is 'e'
is getting traded for 'r' or an 'ap_brigade_send_something()' call is being
substituted for equivalent 'ap_r_send_something()' call. Seems kinda pointless
since 'e' COULD be 'passed around' in 'r' itself and all ap_r_send_something()
calls would still work 'as is'.

( NOTE: Embedded HTML removed or masked in places so mail   )
( programs don't get confused reading this file. Actual     )
( embedded HTML code is irrelevant to the point being made. )

Here are the 'changes only' broken out for reading...

-AP_DECLARE(void) ap_send_size(apr_ssize_t size, request_rec *r);
+AP_DECLARE(void) ap_send_size(apr_ssize_t size, ap_bucket_brigade *b);

-static void emit_preamble(request_rec *r, char *title)
+static void emit_preamble(ap_bucket_brigade *b, char *title)

-    ap_rvputs(r, DOCTYPE_HTML_3_2,
+    ap_brigade_putstrs(b, DOCTYPE_HTML_3_2,

-static void do_emit_plain(request_rec *r, apr_file_t *f)
+static void do_emit_plain(ap_bucket_brigade *e, apr_file_t *f)

-    ap_rputs("\n", r);
+    ap_brigade_puts(e, "\n");

-       ap_rputs(&buf[c], r);
+       ap_brigade_puts(e, &buf[c]);

-       ap_rputs("<", r);
+       ap_brigade_puts(e, "<");

-       ap_rputs(">", r);
+       ap_brigade_puts(e, ">");

-       ap_rputs("&", r);
+       ap_brigade_puts(e, "&");

-    ap_rputs("\n", r);
+    ap_brigade_puts(e, "\n");
 
-static void emit_head(request_rec *r, char *header_fname, int suppress_amble,
-             char *title)
+static void emit_head(request_rec *r, ap_bucket_brigade *e, char 
*header_fname,
+                      int suppress_amble, char *title)

-           emit_preamble(r, title);
+           emit_preamble(e, title);

-           emit_preamble(r, title);
+           emit_preamble(e, title);

-           do_emit_plain(r, f);
+           do_emit_plain(e, f);

-   emit_preamble(r, title);
+   emit_preamble(e, title);

-   ap_rvputs(r, "Index of ", title, "\n", NULL);
+   ap_brigade_putstrs(e, "Index of ", title, "\n", NULL);

-static void emit_tail(request_rec *r, char *readme_fname, int suppress_amble)
+static void emit_tail(request_rec *r, ap_bucket_brigade *e, 
+                      char *readme_fname, int suppress_amble)

-           do_emit_plain(r, f);
+           do_emit_plain(e, f);

-   ap_rputs(ap_psignature("", r), r);
+   ap_brigade_puts(e, ap_psignature("", r));

-   ap_rputs("\n", r);
+   ap_brigade_puts(e, "\n");

-static void emit_link(request_rec *r, char *anchor, char fname, char curkey,
-                      char curdirection, int nosort)
+static void emit_link(request_rec *r, ap_bucket_brigade *e, char *anchor, 
+                      char fname, char curkey, char curdirection, int nosort)

-   ap_rvputs(r, "", anchor,"", NULL);
+   ap_brigade_putstrs(e, "", anchor, "", NULL);

-        ap_rputs(anchor, r);
+        ap_brigade_puts(e, anchor);

 static void output_directories(struct ent **ar, int n,
                   autoindex_config_rec *d, request_rec *r,
-                  int autoindex_opts, char keyid, char direction)
+                  int autoindex_opts, char keyid, char direction,
+                               ap_bucket_brigade *e)

-   ap_rputs("", r);
+   ap_brigade_puts(e, "");

-       ap_rvputs(r, "", ap_escape_html(scratch, tp),
+       ap_brigade_putstrs(e, "", ap_escape_html(scratch, tp),

-       ap_rprintf
+       ap_brigade_printf
            (
-           r,
+           e,
            "",
            d->icon_height,
            d->icon_width
            );
        }

-       ap_rputs("] ", r);
+       ap_brigade_puts(e, "] ");

-        emit_link(r, "Name", K_NAME, keyid, direction, static_columns);
-   ap_rputs(pad_scratch + 4, r);
+        emit_link(r, e, "Name", K_NAME, keyid, direction, static_columns);
+   ap_brigade_puts(e, pad_scratch + 4);

-   ap_rputs(" ", r);
+   ap_brigade_puts(e, " ");

    if (!(autoindex_opts & SUPPRESS_LAST_MOD)) {
-            emit_link(r, "Last modified", K_LAST_MOD, keyid, direction,
+            emit_link(r, e, "Last modified", K_LAST_MOD, keyid, direction,
                       static_columns);

-       ap_rputs("       ", r);
+       ap_brigade_puts(e, "       ");

    if (!(autoindex_opts & SUPPRESS_SIZE)) {
-            emit_link(r, "Size", K_SIZE, keyid, direction, static_columns);
-       ap_rputs("  ", r);
+            emit_link(r, e, "Size", K_SIZE, keyid, direction, 
static_columns);
+       ap_brigade_puts(e, "  ");

    if (!(autoindex_opts & SUPPRESS_DESC)) {
-            emit_link(r, "Description", K_DESC, keyid, direction,
+            emit_link(r, e, "Description", K_DESC, keyid, direction,
                       static_columns);

-   ap_rputs("\n\n", r);
+   ap_brigade_puts(e, "\n\n");

-   ap_rputs("[UL]", r);
+   ap_brigade_puts(e, "[UL]");

    if (autoindex_opts & FANCY_INDEXING) {
        if (autoindex_opts & ICONS_ARE_LINKS) {
-       ap_rvputs(r, "[A HREF="an"]", NULL);
+       ap_brigade_putstrs(e, "[/A][A HREF="an"]", NULL);

        if ((ar[x]->icon) || d->default_icon) {
-       ap_rvputs(r, "(IMG SRC",
+       ap_brigade_putstrs(e, "(IMG SRC",

        if (d->icon_width && d->icon_height) {
-           ap_rprintf(r, " HEIGHT=\"%d\" WIDTH=\"%d\"",
+           ap_brigade_printf(e, " HEIGHT=\"%d\" WIDTH=\"%d\"",
                   d->icon_height, d->icon_width);

-       ap_rputs("]", r);
+       ap_brigade_puts(e, "]");

        if (autoindex_opts & ICONS_ARE_LINKS) {
-       ap_rputs("[/A]", r);
+       ap_brigade_puts(e, "");
 
-       ap_rvputs(r, " [A HREF="an"]",
+       ap_brigade_putstrs(e, " [/A][A HREF="an"]",
          ap_escape_html(scratch, t2), "[/A]", pad_scratch + nwidth,
          NULL);

-       ap_rputs(" ", r);
+       ap_brigade_puts(e, " ");

-           ap_rputs(time_str, r);
+           ap_brigade_puts(e, time_str);

-           ap_rputs("                   ", r);
+           ap_brigade_puts(e, "                   ");

-       ap_send_size(ar[x]->size, r);
-       ap_rputs("  ", r);
+       ap_send_size(ar[x]->size, e);
+       ap_brigade_puts(e, "  ");

-           ap_rputs(terminate_description(d, ar[x]->desc,
-                          autoindex_opts), r);
+           ap_brigade_puts(e, terminate_description(d, ar[x]->desc,
+                          autoindex_opts));

-       ap_rvputs(r, "[LI][A HREF="an"] ", t2,
+       ap_brigade_putstrs(e, "[LI][/A][A HREF="an"] ", t2,

-   ap_rputc('\n', r);
+   ap_brigade_putc(e, '\n');

     if (autoindex_opts & FANCY_INDEXING) {
-   ap_rputs("", r);
+   ap_brigade_puts(e, "");

-   ap_rputs("[/UL]", r);
+   ap_brigade_puts(e, "[/UL]");

 
-static int index_directory(request_rec *r,
+static int index_directory(request_rec *r, ap_bucket_brigade *e,
               autoindex_config_rec *autoindex_conf)

-    emit_head(r, find_header(autoindex_conf, r),
+    emit_head(r, e, find_header(autoindex_conf, r),

     output_directories(ar, num_ent, autoindex_conf, r, autoindex_opts, keyid,
-              direction);
+              direction, e);
 
-   ap_rputs("\n", r);
+   ap_brigade_puts(e, "\n");

-    emit_tail(r, find_readme(autoindex_conf, r),
+    emit_tail(r, e, find_readme(autoindex_conf, r),
 

**** CREATE BRIGADE
**** This could happen 'auto-magically' in ap_rXXXX()
**** calls themselves and 'e' gets stored in 'request
**** record for use throughout the lifetime of the
**** connection. ap_rXXXX() calls just need to be 'smart'
**** enough to use it.
+    ap_bucket_brigade *e = ap_brigade_create(r->pool);
 
     if (allow_opts & OPT_INDEXES) {
+        apr_status_t status;
    }

-   return index_directory(r, d);
+   status = index_directory(r, e, d);
+        if (status == 0) {
+            return ap_pass_brigade(r->output_filters, e);
+        }
+        else {
+            return status;
+        }

+        ap_brigade_standardize(bb);

-AP_DECLARE(void) ap_send_size(apr_ssize_t size, request_rec *r)
+AP_DECLARE(void) ap_send_size(apr_ssize_t size, ap_bucket_brigade *b)

-   ap_rputs("    -", r);
+   ap_brigade_puts(b, "    -");

-   ap_rputs("   0k", r);
+   ap_brigade_puts(b, "   0k");

-   ap_rputs("   1k", r);
+   ap_brigade_puts(b, "   1k");

-   ap_rprintf(r, "%4" APR_SSIZE_T_FMT "k", (size + 512) / 1024);
+   ap_brigade_printf(b, "%4" APR_SSIZE_T_FMT "k", (size + 512) / 1024);

-   ap_rprintf(r, "%4.1fM", size / 1048576.0);
+   ap_brigade_printf(b, "%4.1fM", size / 1048576.0);

-   ap_rprintf(r, "%4" APR_SSIZE_T_FMT "M", (size + 524288) / 1048576);
+   ap_brigade_printf(b, "%4" APR_SSIZE_T_FMT "M", (size + 524288) / 1048576);

* The rest of the patch were changes being made to the low-level
* bucket API's themselves and not mod_autoindex specific.

END OF FILE


Mime
View raw message