You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Paul J. Reder" <re...@raleigh.ibm.com> on 2000/11/20 16:15:23 UTC
[Patch]: mod_include.c port to 2.0 buckets (includes new mod_include.h)
Finally, The promised port/rewrite of mod_include. Sorry it took so long.
This is a CVS diff from locus. It includes the new mod_include.h file
after the mod_include.c diff listing. Sorry it is so long, but it was
a rewrite ;)
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
----------------------------------------------------------------------------
Index: standard/mod_include.c
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/modules/standard/mod_include.c,v
retrieving revision 1.74
diff -r1.74 mod_include.c
102c102
< #include "ap_mpm.h"
---
> #include "mod_include.h"
115,135d114
< #define STARTING_SEQUENCE "<!--#"
< #define ENDING_SEQUENCE "-->"
<
< #define DEFAULT_ERROR_MSG "[an error occurred while processing this directive]"
< #define DEFAULT_TIME_FORMAT "%A, %d-%b-%Y %H:%M:%S %Z"
< #define SIZEFMT_BYTES 0
< #define SIZEFMT_KMG 1
< #ifdef CHARSET_EBCDIC
< #define RAW_ASCII_CHAR(ch) apr_xlate_conv_byte(ap_hdrs_from_ascii, (unsigned char)ch)
< #else /*CHARSET_EBCDIC*/
< #define RAW_ASCII_CHAR(ch) (ch)
< #endif /*CHARSET_EBCDIC*/
<
< module AP_MODULE_DECLARE_DATA includes_module;
<
< /* just need some arbitrary non-NULL pointer which can't also be a request_rec */
< #define NESTED_INCLUDE_MAGIC (&includes_module)
<
< /* TODO: changing directory should be handled by CreateProcess */
< #define ap_chdir_file(x) do {} while(0)
<
184,186c163,168
< #define OUTBUFSIZE 4096
<
< static ap_bucket *find_string(ap_bucket *dptr, const char *str, ap_bucket *end)
---
> /* This function returns either a pointer to the split bucket containing the
> * first byte of the BEGINNING_SEQUENCE (after finding a complete match) or it
> * returns NULL if no match found.
> */
> static ap_bucket *find_start_sequence(ap_bucket *dptr, include_ctx_t *ctx,
> ap_bucket *end, int *do_cleanup)
188c170
< apr_size_t len;
---
> apr_ssize_t len;
191c173,175
< int state = 0;
---
> const char *str = STARTING_SEQUENCE;
>
> *do_cleanup = 0;
204,205c188,194
< if (*c == str[state]) {
< state++;
---
> if (*c == str[ctx->parse_pos]) {
> if (ctx->state == PRE_HEAD) {
> ctx->state = PARSE_HEAD;
> ctx->head_start_bucket = dptr;
> ctx->head_start_index = c - buf;
> }
> ctx->parse_pos++;
208,211c197,224
< if (str[state] == '\0') {
< /* We want to split the bucket at the '<' and '>'
< * respectively. That means adjusting where we split based
< * on what we are searching for.
---
> if (str[ctx->parse_pos] == '\0') {
> ap_bucket *tmp_bkt;
> apr_ssize_t start_index;
>
> /* We want to split the bucket at the '<'. */
> ctx->state = PARSE_TAG;
> ctx->tag_length = 0;
> ctx->parse_pos = 0;
> ctx->tag_start_bucket = dptr;
> ctx->tag_start_index = c - buf;
> if (ctx->head_start_index > 0) {
> start_index = (c - buf) - ctx->head_start_index;
> ap_bucket_split(ctx->head_start_bucket, ctx->head_start_index);
> tmp_bkt = AP_BUCKET_NEXT(ctx->head_start_bucket);
> if (dptr == ctx->head_start_bucket) {
> ctx->tag_start_bucket = tmp_bkt;
> ctx->tag_start_index = start_index;
> }
> ctx->head_start_bucket = tmp_bkt;
> ctx->head_start_index = 0;
> }
> return ctx->head_start_bucket;
> }
> else if (ctx->parse_pos != 0) {
> /* The reason for this, is that we need to make sure
> * that we catch cases like <<!--#. This makes the
> * second check after the original check fails.
> * If parse_pos was already 0 then we already checked this.
213,214c226,231
< if (str[0] == '<') {
< ap_bucket_split(dptr, c - buf - strlen(str));
---
> *do_cleanup = 1;
> if (*c == str[0]) {
> ctx->parse_pos = 1;
> ctx->state = PARSE_HEAD;
> ctx->head_start_bucket = dptr;
> ctx->head_start_index = c - buf;
217c234,289
< ap_bucket_split(dptr, c - buf);
---
> ctx->parse_pos = 0;
> ctx->state = PRE_HEAD;
> ctx->head_start_bucket = NULL;
> ctx->head_start_index = 0;
> }
> }
> }
> c++;
> }
> dptr = AP_BUCKET_NEXT(dptr);
> } while (AP_BUCKET_PREV(dptr) != end);
> return NULL;
> }
>
> static ap_bucket *find_end_sequence(ap_bucket *dptr, include_ctx_t *ctx, ap_bucket *end)
> {
> apr_ssize_t len;
> const char *c;
> const char *buf;
> const char *str = ENDING_SEQUENCE;
>
> do {
> if (AP_BUCKET_IS_EOS(dptr)) {
> break;
> }
> ap_bucket_read(dptr, &buf, &len, 0);
> /* XXX handle retcodes */
> if (len == 0) { /* end of pipe? */
> break;
> }
> if (dptr == ctx->tag_start_bucket) {
> c = buf + ctx->tag_start_index;
> }
> else {
> c = buf;
> }
> while (c - buf != len) {
> if (*c == str[ctx->parse_pos]) {
> if (ctx->state == PARSE_TAG) {
> ctx->state = PARSE_TAIL;
> ctx->tail_start_bucket = dptr;
> ctx->tail_start_index = c - buf;
> }
> ctx->parse_pos++;
> }
> else {
> if (ctx->state == PARSE_TAG) {
> if (ctx->tag_length == 0) {
> if (!apr_isspace(*c)) {
> ctx->tag_start_bucket = dptr;
> ctx->tag_start_index = c - buf;
> ctx->tag_length = 1;
> }
> }
> else {
> ctx->tag_length++;
219d290
< return AP_BUCKET_NEXT(dptr);
222,229c293,327
< state = 0;
< /* The reason for this, is that we need to make sure
< * that we catch cases like <<--#. This makes the
< * second check after the original check fails.
< */
< if (*c == str[state]) {
< state++;
< }
---
> if (str[ctx->parse_pos] == '\0') {
> ap_bucket *tmp_buck = dptr;
>
> /* We want to split the bucket at the '>'. The
> * end of the END_SEQUENCE is in the current bucket.
> * The beginning might be in a previous bucket.
> */
> ctx->state = PARSED;
> if ((c - buf) > 0) {
> ap_bucket_split(dptr, c - buf);
> tmp_buck = AP_BUCKET_NEXT(dptr);
> }
> return (tmp_buck);
> }
> else if (ctx->parse_pos != 0) {
> /* The reason for this, is that we need to make sure
> * that we catch cases like --->. This makes the
> * second check after the original check fails.
> * If parse_pos was already 0 then we already checked this.
> */
> ctx->tag_length += ctx->parse_pos;
>
> if (*c == str[0]) {
> ctx->parse_pos = 1;
> ctx->state = PARSE_TAIL;
> ctx->tail_start_bucket = dptr;
> ctx->tail_start_index = c - buf;
> }
> else {
> ctx->parse_pos = 0;
> ctx->state = PARSE_TAG;
> ctx->tail_start_bucket = NULL;
> ctx->tail_start_index = 0;
> }
> }
238a337,413
> /* This function culls through the buckets that have been set aside in the
> * ssi_tag_brigade and copies just the directive part of the SSI tag (none
> * of the start and end delimiter bytes are copied). The is_allocated paramter
> * is set according to the following algorithm:
> * 0 : The combined directive resides in a single bucket (the combined_tag
> * pointer is set to the beginning of the directive in the bucket), or
> * the combined directive will fit in the local stack buffer.
> * 1 : The combined directive was too big to fit in the local stack buffer.
> * A buffer is allocated and the combined directive is copied into it.
> */
> static apr_status_t get_combined_directive (include_ctx_t *ctx,
> ap_bucket_brigade *bb,
> int *is_allocated,
> char *tmp_buf, int tmp_buf_size)
> {
> int done = 0;
> ap_bucket *dptr, *ctx_end, *bb_end;
> const char *tmp_from;
> apr_ssize_t tmp_from_len;
>
> *is_allocated = 0;
>
> /* If the tag length is longer than the tmp buffer, allocate space. */
> if (ctx->tag_length > tmp_buf_size-1) {
> if ((ctx->combined_tag = malloc (ctx->tag_length + 1)) == NULL) {
> return (APR_ENOMEM);
> }
> else {
> *is_allocated = 1; /* Mark that it is allocated for later freeing. */
> }
> } /* Else, just use the temp buffer. */
> else {
> ctx->combined_tag = tmp_buf;
> }
>
> /* Prime the pump. Start at the beginning of the tag... */
> dptr = ctx->tag_start_bucket;
> ap_bucket_read (dptr, &tmp_from, &tmp_from_len, 0); /* Read the bucket... */
>
> /* Adjust the pointer to start at the tag within the bucket... */
> if (dptr == ctx->tail_start_bucket) {
> tmp_from_len -= (tmp_from_len - ctx->tail_start_index);
> }
> tmp_from = &tmp_from[ctx->tag_start_index];
> tmp_from_len -= ctx->tag_start_index;
> ctx->curr_tag_pos = ctx->combined_tag;
> ctx_end = AP_BRIGADE_LAST(ctx->ssi_tag_brigade);
> bb_end = AP_BRIGADE_LAST(bb);
>
> do { /* Loop through and copy all of the pertinent bucket contents into */
> memcpy (ctx->curr_tag_pos, tmp_from, tmp_from_len); /* the buffer. */
> ctx->curr_tag_pos += tmp_from_len;
>
> if (dptr == ctx->tail_start_bucket) { /* If I just copied the contents*/
> done = 1; /* of the tail_start_bucket */
> } /* then I'm done. */
> else { /* If I reach the end of */
> dptr = AP_BUCKET_NEXT (dptr); /* ssi_tag_brigade then I*/
> if (AP_BUCKET_PREV(dptr) == ctx_end) { /* need to continue with */
> dptr = AP_BRIGADE_FIRST(bb); /* the current brigade, */
> } /* until I find tail_start_bucket...*/
>
> ap_bucket_read (dptr, &tmp_from, &tmp_from_len, 0);
>
> if (dptr == ctx->tail_start_bucket) {
> tmp_from_len -= (tmp_from_len - ctx->tail_start_index);
> }
> }
> } while ((!done) && (AP_BUCKET_PREV(dptr) != bb_end) &&
> ((ctx->curr_tag_pos - ctx->combined_tag) < ctx->tag_length));
>
> ctx->combined_tag[ctx->tag_length] = '\0';
> ctx->curr_tag_pos = ctx->combined_tag;
>
> return (APR_SUCCESS);
> }
>
334,336c509,514
< * extract the next tag name and value.
< * if there are no more tags, set the tag name to 'done'
< * the tag value is html decoded if dodecode is non-zero
---
> * Extract the next tag name and value.
> * If there are no more tags, set the tag name to NULL.
> * The tag value is html decoded if dodecode is non-zero.
> * The tag value may be NULL if there is no tag value..
> * format:
> * [WS]<Tag>[WS]=[WS]['|"]<Value>['|"|WS]
339,345c517
< static char *get_tag(apr_pool_t *p, ap_bucket *in, char *tag, int tagbuf_len, int dodecode, apr_off_t *offset)
< {
< ap_bucket *dptr = in;
< const char *c;
< const char *str;
< apr_size_t length;
< char *t = tag, *tag_val, term;
---
> #define SKIP_TAG_WHITESPACE(ptr) while ((*ptr != '\0') && (apr_isspace (*ptr))) ptr++
347,348c519,523
< /* makes code below a little less cluttered */
< --tagbuf_len;
---
> static char *get_tag_and_value(include_ctx_t *ctx, char **tag, int dodecode)
> {
> char *c = ctx->curr_tag_pos;
> int shift_val = 0;
> char *tag_val = NULL, term = '\0';
350,365c525,526
< /* Remove all whitespace */
< do {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str + *offset;
< *offset = 0;
< while (c - str < length) {
< if (!apr_isspace(*c)) {
< break;
< }
< c++;
< }
< if (!apr_isspace(*c)) {
< break;
< }
< dptr = AP_BUCKET_NEXT(dptr);
< } while (dptr);
---
> SKIP_TAG_WHITESPACE(c);
> *tag = c; /* First non-whitespace character (could be NULL). */
367,368c528,529
< /* tags can't start with - */
< if (*c == '-') {
---
> while ((*c != '\0') && (*c != '=') && (!apr_isspace(*c))) {
> *c = apr_tolower(*c); /* find end of tag, lowercasing as we go... */
370,388d530
< if (c == '\0') {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str;
< }
< if (*c == '-') {
< do {
< c++;
< if (c == '\0') {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str;
< }
< } while (apr_isspace(*c));
< if (*c == '>') {
< apr_cpystrn(tag, "done", tagbuf_len);
< *offset = c - str;
< return tag;
< }
< }
< return NULL; /* failed */
391,404c533,550
< /* find end of tag name */
< while (1) {
< if (t - tag == tagbuf_len) {
< *t = '\0';
< return NULL;
< }
< if (*c == '=' || apr_isspace(*c)) {
< break;
< }
< *(t++) = apr_tolower(*c);
< c++;
< if (c == '\0') {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str;
---
> if ((*c == '\0') || (**tag == '=')) {
> if ((**tag == '\0') || (**tag == '=')) {
> *tag = NULL;
> }
> ctx->curr_tag_pos = c;
> return NULL; /* We have found the end of the buffer. */
> } /* We might have a tag, but definitely no value. */
>
> if (*c == '=') {
> *c++ = '\0'; /* Overwrite the '=' with a terminating byte after tag. */
> }
> else { /* Try skipping WS to find the '='. */
> *c++ = '\0'; /* Terminate the tag... */
> SKIP_TAG_WHITESPACE(c);
>
> if (*c != '=') { /* There needs to be an equal sign if there's a value. */
> ctx->curr_tag_pos = c;
> return NULL; /* There apparently was no value. */
406,415c552,553
< }
<
< *t++ = '\0';
< tag_val = t;
<
< while (apr_isspace(*c)) {
< c++;
< if (c == '\0') {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str;
---
> else {
> c++; /* Skip the equals sign. */
418,421d555
< if (*c != '=') {
< /* XXX may need to ungetc() here (see pre-bucketized code) */
< return NULL;
< }
423,434c557,559
< do {
< c++;
< if (c == '\0') {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str;
< }
< } while (apr_isspace(*c));
<
< /* we should allow a 'name' as a value */
<
< if (*c != '"' && *c != '\'') {
< return NULL;
---
> SKIP_TAG_WHITESPACE(c);
> if (*c == '"' || *c == '\'') { /* Allow quoted values for space inclusion. */
> term = *c++; /* NOTE: This does not pass the quotes on return. */
436,449c561,566
< term = *c;
< while (1) {
< c++;
< if (c == '\0') {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str;
< }
< if (t - tag == tagbuf_len) {
< *t = '\0';
< return NULL;
< }
< /* Want to accept \" as a valid character within a string. */
< if (*c == '\\') {
< *(t++) = *c; /* Add backslash */
---
>
> tag_val = c;
> while ((*c != '\0') &&
> (((term != '\0') && (*c != term)) ||
> ((term == '\0') && (!apr_isspace(*c))))) {
> if (*c == '\\') { /* Accept \" and \' as valid char in string. */
451,456c568,572
< if (c == '\0') {
< ap_bucket_read(dptr, &str, &length, 0);
< c = str;
< }
< if (*c == term) { /* Only if */
< *(--t) = *c; /* Replace backslash ONLY for terminator */
---
> if (*c == term) { /* Overwrite the "\" during the embedded */
> shift_val++; /* escape sequence of '\"' or "\'". Shift */
> } /* bytes from here to next delimiter. */
> if (shift_val > 0) {
> *(c-shift_val) = *c;
459,460c575,578
< else if (*c == term) {
< break;
---
>
> c++;
> if (shift_val > 0) {
> *(c-shift_val) = *c;
462d579
< *(t++) = *c;
464c581,583
< *t = '\0';
---
>
> *c++ = '\0'; /* Overwrites delimiter (term or WS) with NULL. */
> ctx->curr_tag_pos = c;
468,469c587,588
< *offset = c - str + 1;
< return apr_pstrdup(p, tag_val);
---
>
> return (tag_val);
472c591
< static int get_directive(ap_bucket *in, char *dest, size_t len, apr_pool_t *p)
---
> static char *get_directive(include_ctx_t *ctx, dir_token_id *fnd_token)
474,478c593,595
< ap_bucket *dptr = in;
< char *d = dest;
< const char *c;
< const char *str;
< apr_size_t length;
---
> char *c = ctx->curr_tag_pos;
> char *dest;
> int len = 0;
480,481c597
< /* make room for nul terminator */
< --len;
---
> SKIP_TAG_WHITESPACE(c);
483,496c599,603
< while (dptr) {
< ap_bucket_read(dptr, &str, &length, 0);
< /* need to start past the <!--#
< */
< c = str + strlen(STARTING_SEQUENCE);
< while (c - str < length) {
< if (!apr_isspace(*c)) {
< break;
< }
< }
< if (!apr_isspace(*c)) {
< break;
< }
< dptr = AP_BUCKET_NEXT(dptr);
---
> dest = c;
> /* now get directive */
> while ((*c != '\0') && (!apr_isspace(*c))) {
> *c++ = apr_tolower(*c);
> len++;
497a605,607
>
> *c++ = '\0';
> ctx->curr_tag_pos = c;
499,514c609,631
< /* now get directive */
< while (dptr) {
< if (c - str >= length) {
< ap_bucket_read(dptr, &str, &length, 0);
< }
< while (c - str < length) {
< if (d - dest == (int)len) {
< return 1;
< }
< *d++ = apr_tolower(*c);
< c++;
< if (apr_isspace(*c)) {
< break;
< }
< }
< if (apr_isspace(*c)) {
---
> *fnd_token = TOK_UNKNOWN;
> switch (len) {
> case 2: if (!strcmp(dest, "if")) *fnd_token = TOK_IF;
> break;
> case 3: if (!strcmp(dest, "set")) *fnd_token = TOK_SET;
> break;
> case 4: if (!strcmp(dest, "else")) *fnd_token = TOK_ELSE;
> else if (!strcmp(dest, "elif")) *fnd_token = TOK_ELIF;
> else if (!strcmp(dest, "exec")) *fnd_token = TOK_EXEC;
> else if (!strcmp(dest, "echo")) *fnd_token = TOK_ECHO;
> #ifdef USE_PERL_SSI
> else if (!strcmp(dest, "perl")) *fnd_token = TOK_PERL;
> #endif
> break;
> case 5: if (!strcmp(dest, "endif")) *fnd_token = TOK_ENDIF;
> else if (!strcmp(dest, "fsize")) *fnd_token = TOK_FSIZE;
> break;
> case 6: if (!strcmp(dest, "config")) *fnd_token = TOK_CONFIG;
> break;
> case 7: if (!strcmp(dest, "include")) *fnd_token = TOK_INCLUDE;
> break;
> case 8: if (!strcmp(dest, "flastmod")) *fnd_token = TOK_FLASTMOD;
> else if (!strcmp(dest, "printenv")) *fnd_token = TOK_PRINTENV;
516,517d632
< }
< dptr = AP_BUCKET_NEXT(dptr);
519,520c634,635
< *d = '\0';
< return 0;
---
>
> return (dest);
553c668
< char var[MAX_STRING_LEN];
---
> /* pjr hack char var[MAX_STRING_LEN]; */
555c670
< const char *end_of_var_name; /* end of var name + 1 */
---
> char *end_of_var_name; /* end of var name + 1 */
557a673
> char tmp_store;
573c689
< end_of_var_name = in;
---
> (const char *)end_of_var_name = in;
581c697
< end_of_var_name = in;
---
> (const char *)end_of_var_name = in;
587,589c703,713
< l = (l > sizeof(var) - 1) ? (sizeof(var) - 1) : l;
< memcpy(var, start_of_var_name, l);
< var[l] = '\0';
---
> /* pjr - this is a test hack to avoid a memcpy. Make sure that this works...
> * l = (l > sizeof(var) - 1) ? (sizeof(var) - 1) : l;
> * memcpy(var, start_of_var_name, l);
> * var[l] = '\0';
> *
> * val = apr_table_get(r->subprocess_env, var);
> */
> /* pjr hack */ tmp_store = *end_of_var_name;
> /* pjr hack */ *end_of_var_name = '\0';
> /* pjr hack */ val = apr_table_get(r->subprocess_env, start_of_var_name);
> /* pjr hack */ *end_of_var_name = tmp_store;
591d714
< val = apr_table_get(r->subprocess_env, var);
628c751,752
< static int include_cgi(char *s, request_rec *r, ap_filter_t *next)
---
> static int include_cgi(char *s, request_rec *r, ap_filter_t *next,
> ap_bucket *head_ptr, ap_bucket **inserted_head)
631a756
> ap_bucket *tmp_buck, *tmp2_buck;
663a789
> apr_ssize_t len_loc;
664a791
>
666c793,808
< ap_rvputs(r, "<A HREF=\"", location, "\">", location, "</A>", NULL);
---
> len_loc = strlen(location);
>
> tmp_buck = ap_bucket_create_immortal("<A HREF=\"", sizeof("<A HREF=\""));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> tmp2_buck = ap_bucket_create_transient(location, len_loc);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_immortal("\">", sizeof("\">"));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_transient(location, len_loc);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_immortal("</A>", sizeof("</A>"));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
>
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
715,716c857,858
< static int handle_include(ap_bucket *in, request_rec *r, ap_filter_t *next,
< const char *error, int noexec)
---
> static int handle_include(include_ctx_t *ctx, request_rec *r, ap_filter_t *next,
> ap_bucket *head_ptr, ap_bucket **inserted_head)
718c860,862
< char tag[MAX_STRING_LEN];
---
> char *tag = NULL;
> char *tag_val = NULL;
> ap_bucket *tmp_buck;
720,721d863
< char *tag_val;
< apr_off_t offset = strlen("include ") + strlen(STARTING_SEQUENCE);
722a865
> *inserted_head = NULL;
724,725c867,873
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
< return 1;
---
> if (!(tag_val = get_tag_and_value(ctx, &tag, 1))) {
> if (tag == NULL) {
> return (0);
> }
> else {
> return (1);
> }
750c898
< if (!error_fmt && noexec && rr->content_type
---
> if (!error_fmt && (ctx->flags & FLAG_NO_EXEC) && rr->content_type
797c945,947
< /* see the Kludge in send_parsed_file for why */
---
> /* See the Kludge in send_parsed_file for why */
> /* Basically, it puts a bread crumb in here, then looks */
> /* for the crumb later to see if its been here. */
813c963
< ap_rputs(error, r);
---
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
816c966
< /* destroy the sub request if it's not a nested include */
---
> /* destroy the sub request if it's not a nested include (crumb) */
823,825d972
< else if (!strcmp(tag, "done")) {
< return 0;
< }
830c977
< ap_rputs(error, r);
---
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
951c1098
< rc = ap_os_create_privileged_process(r, procnew, s, argv, ap_create_environment(r->pool, env), procattr, r->pool);
---
> rc = apr_create_process(procnew, s, argv, ap_create_environment(r->pool, env), procattr, r->pool);
982,983c1129,1130
< static int handle_exec(ap_bucket *in, request_rec *r, const char *error,
< ap_filter_t *next)
---
> static int handle_exec(include_ctx_t *ctx, request_rec *r, ap_filter_t *next,
> ap_bucket *head_ptr, ap_bucket **inserted_head)
985,986c1132,1133
< char tag[MAX_STRING_LEN];
< char *tag_val;
---
> char *tag = NULL;
> char *tag_val = NULL;
987a1135
> ap_bucket *tmp_buck;
989d1136
< apr_off_t offset = strlen("exec ") + strlen(STARTING_SEQUENCE);
990a1138
> *inserted_head = NULL;
992,993c1140,1146
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
< return 1;
---
> if (!(tag_val = get_tag_and_value(ctx, &tag, 1))) {
> if (tag == NULL) {
> return (0);
> }
> else {
> return 1;
> }
1000,1002c1153,1154
< "to tag exec in file %s",
< tag, r->filename);
< ap_rputs(error, r);
---
> "to tag exec in file %s", tag, r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1009c1161
< if (include_cgi(parsed_string, r, next) == -1) {
---
> if (include_cgi(parsed_string, r, next, head_ptr, inserted_head) == -1) {
1012c1164
< ap_rputs(error, r);
---
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1016,1018d1167
< else if (!strcmp(tag, "done")) {
< return 0;
< }
1021,1023c1170,1171
< "unknown parameter \"%s\" to tag exec in %s",
< tag, file);
< ap_rputs(error, r);
---
> "unknown parameter \"%s\" to tag exec in %s", tag, file);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1029c1177,1178
< static int handle_echo(ap_bucket *in, request_rec *r, const char *error)
---
> static int handle_echo(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
1031,1032c1180,1184
< char tag[MAX_STRING_LEN];
< char *tag_val;
---
> char *tag = NULL;
> char *tag_val = NULL;
> const char *echo_text = NULL;
> ap_bucket *tmp_buck;
> apr_ssize_t e_len;
1034d1185
< apr_off_t offset = strlen("echo ") + strlen(STARTING_SEQUENCE);
1037a1189
> *inserted_head = NULL;
1039,1040c1191,1197
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
< return 1;
---
> if (!(tag_val = get_tag_and_value(ctx, &tag, 1))) {
> if (tag != NULL) {
> return (1);
> }
> else {
> return (0);
> }
1043a1201
> int b_copy = 0;
1046,1053c1204,1207
< if (encode == E_NONE) {
< ap_rputs(val, r);
< }
< else if (encode == E_URL) {
< ap_rputs(ap_escape_uri(r->pool, val), r);
< }
< else if (encode == E_ENTITY) {
< ap_rputs(ap_escape_html(r->pool, val), r);
---
> switch(encode) {
> case E_NONE: echo_text = val; b_copy = 1; break;
> case E_URL: echo_text = ap_escape_uri(r->pool, val); break;
> case E_ENTITY: echo_text = ap_escape_html(r->pool, val); break;
1054a1209,1211
>
> e_len = strlen(echo_text);
> tmp_buck = ap_bucket_create_transient(echo_text, e_len);
1057c1214,1218
< ap_rputs("(none)", r);
---
> tmp_buck = ap_bucket_create_immortal("(none)", sizeof("none"));
> }
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
1059,1061d1219
< }
< else if (!strcmp(tag, "done")) {
< return 0;
1070,1072c1228,1229
< "tag echo in %s",
< tag_val, r->filename);
< ap_rputs(error, r);
---
> "tag echo in %s", tag_val, r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1075d1231
<
1078c1234
< "unknown parameter \"%s\" to tag echo in %s",
---
> "unknown parameter \"%s\" in tag echo of %s",
1080c1236
< ap_rputs(error, r);
---
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1081a1238
>
1086c1243
< static int handle_perl(ap_bucket *in, request_rec *r, const char *error)
---
> static int handle_perl(include_ctx_t *ctx, request_rec *r)
1088c1245,1246
< char tag[MAX_STRING_LEN];
---
> char *tag = NULL;
> char *tag_val = NULL;
1090d1247
< char *tag_val;
1093d1249
< apr_off_t offset = strlen("perl ") + strlen(STARTING_SEQUENCE);
1095c1251
< if (ap_allow_options(r) & OPT_INCNOEXEC) {
---
> if (ctx->flags & FLAG_NO_EXEC) {
1099c1255
< return DECLINED;
---
> return (DECLINED);
1100a1257
>
1102c1259
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
---
> if (!(tag_val = get_tag_and_value(ctx, &tag, 1))) {
1112,1114d1268
< else if (strnEQ(tag, "done", 4)) {
< break;
< }
1115a1270
>
1119c1274,1275
< return OK;
---
>
> return (OK);
1126,1127c1282,1283
< static int handle_config(ap_bucket *in, request_rec *r, char *error, char *tf,
< int *sizefmt)
---
> static int handle_config(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
1129,1130c1285,1286
< char tag[MAX_STRING_LEN];
< char *tag_val;
---
> char *tag = NULL;
> char *tag_val = NULL;
1133d1288
< apr_off_t offset = strlen("config ") + strlen(STARTING_SEQUENCE);
1134a1290
> *inserted_head = NULL;
1136,1137c1292,1298
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 0, &offset))) {
< return 1;
---
> if (!(tag_val = get_tag_and_value(ctx, &tag, 0))) {
> if (tag == NULL) {
> return 0; /* Reached the end of the string. */
> }
> else {
> return 1; /* tags must have values. */
> }
1140c1301,1302
< parse_string(r, tag_val, error, MAX_STRING_LEN, 0);
---
> parse_string(r, tag_val, ctx->error_str, MAX_STRING_LEN, 0);
> ctx->error_length = strlen(ctx->error_str);
1145,1147c1307,1309
< parse_string(r, tag_val, tf, MAX_STRING_LEN, 0);
< apr_table_setn(env, "DATE_LOCAL", ap_ht_time(r->pool, date, tf, 0));
< apr_table_setn(env, "DATE_GMT", ap_ht_time(r->pool, date, tf, 1));
---
> parse_string(r, tag_val, ctx->time_str, MAX_STRING_LEN, 0);
> apr_table_setn(env, "DATE_LOCAL", ap_ht_time(r->pool, date, ctx->time_str, 0));
> apr_table_setn(env, "DATE_GMT", ap_ht_time(r->pool, date, ctx->time_str, 1));
1149c1311
< ap_ht_time(r->pool, r->finfo.mtime, tf, 0));
---
> ap_ht_time(r->pool, r->finfo.mtime, ctx->time_str, 0));
1155c1317
< *sizefmt = SIZEFMT_BYTES;
---
> ctx->flags |= FLAG_SIZE_IN_BYTES;
1158c1320
< *sizefmt = SIZEFMT_KMG;
---
> ctx->flags &= FLAG_SIZE_ABBREV;
1161,1163d1322
< else if (!strcmp(tag, "done")) {
< return 0;
< }
1164a1324,1325
> ap_bucket *tmp_buck;
>
1168c1329
< ap_rputs(error, r);
---
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1175c1336
< char *tag_val, apr_finfo_t *finfo, const char *error)
---
> char *tag_val, apr_finfo_t *finfo)
1211d1371
< ap_rputs(error, r);
1232d1391
< ap_rputs(error, r);
1241d1399
< ap_rputs(error, r);
1245a1404,1429
> #define NEG_SIGN " -"
> #define ZERO_K " 0k"
> #define ONE_K " 1k"
>
> static void generate_size(apr_ssize_t size, char *buff, apr_ssize_t buff_size)
> {
> /* XXX: this -1 thing is a gross hack */
> if (size == (apr_ssize_t)-1) {
> memcpy (buff, NEG_SIGN, sizeof(NEG_SIGN)+1);
> }
> else if (!size) {
> memcpy (buff, ZERO_K, sizeof(ZERO_K)+1);
> }
> else if (size < 1024) {
> memcpy (buff, ONE_K, sizeof(ONE_K)+1);
> }
> else if (size < 1048576) {
> apr_snprintf(buff, buff_size, "%4" APR_SSIZE_T_FMT "k", (size + 512) / 1024);
> }
> else if (size < 103809024) {
> apr_snprintf(buff, buff_size, "%4.1fM", size / 1048576.0);
> }
> else {
> apr_snprintf(buff, buff_size, "%4" APR_SSIZE_T_FMT "M", (size + 524288) / 1048576);
> }
> }
1247c1431,1432
< static int handle_fsize(ap_bucket *in, request_rec *r, const char *error, int sizefmt)
---
> static int handle_fsize(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
1249,1251c1434,1438
< char tag[MAX_STRING_LEN];
< char *tag_val;
< apr_finfo_t finfo;
---
> char *tag = NULL;
> char *tag_val = NULL;
> apr_finfo_t finfo;
> apr_ssize_t s_len, s_wrt;
> ap_bucket *tmp_buck;
1253d1439
< apr_off_t offset = strlen("fsize ") + strlen(STARTING_SEQUENCE);
1254a1441
> *inserted_head = NULL;
1256,1260c1443,1449
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
< return 1;
< }
< else if (!strcmp(tag, "done")) {
< return 0;
---
> if (!(tag_val = get_tag_and_value(ctx, &tag, 1))) {
> if (tag == NULL) {
> return 0;
> }
> else {
> return 1;
> }
1264,1266c1453,1458
< if (!find_file(r, "fsize", tag, parsed_string, &finfo, error)) {
< if (sizefmt == SIZEFMT_KMG) {
< ap_send_size(finfo.size, r);
---
> if (!find_file(r, "fsize", tag, parsed_string, &finfo)) {
> char buff[50];
>
> if (!(ctx->flags & FLAG_SIZE_IN_BYTES)) {
> generate_size(finfo.size, buff, sizeof(buff));
> s_len = strlen (buff);
1269,1271c1461,1465
< int l, x;
< apr_snprintf(tag, sizeof(tag), "%" APR_OFF_T_FMT, finfo.size);
< l = strlen(tag); /* grrr */
---
> int l, x, pos = 0;
> char tmp_buff[50];
>
> apr_snprintf(tmp_buff, sizeof(tmp_buff), "%" APR_OFF_T_FMT, finfo.size);
> l = strlen(tmp_buff); /* grrr */
1274c1468
< ap_rputc(',', r);
---
> buff[pos++] = ',';
1276c1470
< ap_rputc(tag[x], r);
---
> buff[pos++] = tmp_buff[x];
1277a1472,1473
> buff[pos] = '\0';
> s_len = pos;
1278a1475,1483
>
> tmp_buck = ap_bucket_create_heap(buff, s_len, 1, &s_wrt);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
> }
> else {
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1284c1489,1490
< static int handle_flastmod(ap_bucket *in, request_rec *r, const char *error, const char *tf)
---
> static int handle_flastmod(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
1286,1288c1492,1496
< char tag[MAX_STRING_LEN];
< char *tag_val;
< apr_finfo_t finfo;
---
> char *tag = NULL;
> char *tag_val = NULL;
> apr_finfo_t finfo;
> apr_ssize_t t_len;
> ap_bucket *tmp_buck;
1290d1497
< apr_off_t offset = strlen("flastmod ") + strlen(STARTING_SEQUENCE);
1291a1499
> *inserted_head = NULL;
1293,1297c1501,1507
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
< return 1;
< }
< else if (!strcmp(tag, "done")) {
< return 0;
---
> if (!(tag_val = get_tag_and_value(ctx, &tag, 1))) {
> if (tag == NULL) {
> return 0;
> }
> else {
> return 1;
> }
1301,1302c1511,1524
< if (!find_file(r, "flastmod", tag, parsed_string, &finfo, error)) {
< ap_rputs(ap_ht_time(r->pool, finfo.mtime, tf, 0), r);
---
> if (!find_file(r, "flastmod", tag, parsed_string, &finfo)) {
> char *t_val;
>
> t_val = ap_ht_time(r->pool, finfo.mtime, ctx->time_str, 0);
> t_len = strlen(t_val);
>
> tmp_buck = ap_bucket_create_transient(t_val, t_len);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
> }
> else {
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
1342a1565
> int tkn_fnd = 0;
1429c1652
< for (ch = *string; ch != '\0'; ch = *++string) {
---
> for (ch = *string; ((ch != '\0') && (!tkn_fnd)); ch = *++string) {
1432c1655,1658
< goto TOKEN_DONE;
---
> tkn_fnd = 1;
> }
> else {
> token->value[next++] = ch;
1434,1435d1659
< token->value[next++] = ch;
< continue;
1437,1452c1661,1664
< if (!qs) {
< if (apr_isspace(ch)) {
< goto TOKEN_DONE;
< }
< switch (ch) {
< case '(':
< goto TOKEN_DONE;
< case ')':
< goto TOKEN_DONE;
< case '=':
< goto TOKEN_DONE;
< case '!':
< goto TOKEN_DONE;
< case '|':
< if (*(string + 1) == '|') {
< goto TOKEN_DONE;
---
> else {
> if (!qs) {
> if (apr_isspace(ch)) {
> tkn_fnd = 1;
1454,1457c1666,1689
< break;
< case '&':
< if (*(string + 1) == '&') {
< goto TOKEN_DONE;
---
> else {
> switch (ch) {
> case '(':
> case ')':
> case '=':
> case '!':
> case '<':
> case '>':
> tkn_fnd = 1;
> break;
> case '|':
> if (*(string + 1) == '|') {
> tkn_fnd = 1;
> }
> break;
> case '&':
> if (*(string + 1) == '&') {
> tkn_fnd = 1;
> }
> break;
> }
> if (!tkn_fnd) {
> token->value[next++] = ch;
> }
1459,1463d1690
< break;
< case '<':
< goto TOKEN_DONE;
< case '>':
< goto TOKEN_DONE;
1465,1471c1692,1700
< token->value[next++] = ch;
< }
< else {
< if (ch == '\'') {
< qs = 0;
< ++string;
< goto TOKEN_DONE;
---
> else {
> if (ch == '\'') {
> qs = 0;
> ++string;
> tkn_fnd = 1;
> }
> else {
> token->value[next++] = ch;
> }
1473d1701
< token->value[next++] = ch;
1476c1704
< TOKEN_DONE:
---
>
2045c2273
< "bad token type");
---
> "bad token type");
2057,2063c2285,2307
< static int handle_if(ap_bucket *in, request_rec *r, const char *error,
< int *conditional_status, int *printing)
< {
< char tag[MAX_STRING_LEN];
< char *tag_val;
< char *expr;
< apr_off_t offset = strlen("if ") + strlen(STARTING_SEQUENCE);
---
> #define LOG_COND_STATUS(cntx, t_buck, h_ptr, ins_head, tag_text) \
> char *cond_txt = "**** X conditional_status=\"0\"\n"; \
> \
> if (cntx->flags & FLAG_COND_TRUE) { \
> cont_txt[31] = '1'; \
> } \
> memcpy(&cond_txt[5], tag_text, sizeof(tag_text)); \
> t_buck = ap_bucket_create_transient(cond_txt, sizeof(cond_txt)); \
> AP_BUCKET_INSERT_BEFORE(h_ptr, t_buck); \
> \
> if (ins_head == NULL) { \
> ins_head = t_buck; \
> }
>
>
> /* pjr - These seem to allow expr="fred" expr="joe" where joe overwrites fred. */
> static int handle_if(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
> {
> char *tag = NULL;
> char *tag_val = NULL;
> char *expr = NULL;
> ap_bucket *tmp_buck;
2065c2309
< expr = NULL;
---
> *inserted_head = NULL;
2067,2071c2311,2312
< tag_val = get_tag(r->pool, in, tag, sizeof(tag), 0, &offset);
< if (*tag == '\0') {
< return 1;
< }
< else if (!strcmp(tag, "done")) {
---
> tag_val = get_tag_and_value(ctx, &tag, 0);
> if (tag == NULL) {
2074,2076c2315,2317
< "missing expr in if statement: %s",
< r->filename);
< ap_rputs(error, r);
---
> "missing expr in if statement: %s", r->filename);
>
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
2079c2320,2325
< *printing = *conditional_status = parse_expr(r, expr, error);
---
> if (parse_expr(r, expr, ctx->error_str)) {
> ctx->flags |= (FLAG_PRINTING | FLAG_COND_TRUE);
> }
> else {
> ctx->flags &= FLAG_CLEAR_PRINT_COND;
> }
2081,2082c2327
< ap_rvputs(r, "**** if conditional_status=\"",
< *conditional_status ? "1" : "0", "\"\n", NULL);
---
> LOG_COND_STATUS(ctx, tmp_buck, head_ptr, *inserted_head, " if");
2089c2334,2344
< ap_rvputs(r, "**** if expr=\"", expr, "\"\n", NULL);
---
> ap_bucket *tmp2_buck;
> tmp_buck = ap_bucket_create_immortal("**** if expr=\"", 15);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> tmp2_buck = ap_bucket_create_transient(expr, strlen(expr));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_immortal("\"\n", sizeof("\"\n");
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
>
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
2094,2096c2349,2350
< "unknown parameter \"%s\" to tag if in %s",
< tag, r->filename);
< ap_rputs(error, r);
---
> "unknown parameter \"%s\" to tag if in %s", tag, r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
2097a2352
>
2101,2102c2356,2357
< static int handle_elif(ap_bucket *in, request_rec *r, const char *error,
< int *conditional_status, int *printing)
---
> static int handle_elif(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
2104,2107c2359,2362
< char tag[MAX_STRING_LEN];
< char *tag_val;
< char *expr;
< apr_off_t offset = strlen("elif ") + strlen(STARTING_SEQUENCE);
---
> char *tag = NULL;
> char *tag_val = NULL;
> char *expr = NULL;
> ap_bucket *tmp_buck;
2109c2364
< expr = NULL;
---
> *inserted_head = NULL;
2111,2115c2366,2367
< tag_val = get_tag(r->pool, in, tag, sizeof(tag), 0, &offset);
< if (*tag == '\0') {
< return 1;
< }
< else if (!strcmp(tag, "done")) {
---
> tag_val = get_tag_and_value(ctx, &tag, 0);
> if (tag == '\0') {
2117,2118c2369
< ap_rvputs(r, "**** elif conditional_status=\"",
< *conditional_status ? "1" : "0", "\"\n", NULL);
---
> LOG_COND_STATUS(ctx, tmp_buck, head_ptr, *inserted_head, " elif");
2120,2121c2371,2372
< if (*conditional_status) {
< *printing = 0;
---
> if (ctx->flags & FLAG_COND_TRUE) {
> ctx->flags &= FLAG_CLEAR_PRINTING;
2126,2129c2377,2379
< "missing expr in elif statement: %s",
< r->filename);
< ap_rputs(error, r);
< return 1;
---
> "missing expr in elif statement: %s", r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
> return (1);
2131c2381,2386
< *printing = *conditional_status = parse_expr(r, expr, error);
---
> if (parse_expr(r, expr, ctx->error_str)) {
> ctx->flags |= (FLAG_PRINTING | FLAG_COND_TRUE);
> }
> else {
> ctx->flags &= FLAG_CLEAR_PRINT_COND;
> }
2133,2134c2388
< ap_rvputs(r, "**** elif conditional_status=\"",
< *conditional_status ? "1" : "0", "\"\n", NULL);
---
> LOG_COND_STATUS(ctx, tmp_buck, head_ptr, *inserted_head, " elif");
2136c2390
< return 0;
---
> return (0);
2141c2395,2405
< ap_rvputs(r, "**** if expr=\"", expr, "\"\n", NULL);
---
> ap_bucket *tmp2_buck;
> tmp_buck = ap_bucket_create_immortal("**** elif expr=\"", 17);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> tmp2_buck = ap_bucket_create_transient(expr, strlen(expr));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_immortal("\"\n", sizeof("\"\n");
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
>
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
2146,2148c2410,2411
< "unknown parameter \"%s\" to tag if in %s",
< tag, r->filename);
< ap_rputs(error, r);
---
> "unknown parameter \"%s\" to tag if in %s", tag, r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
2153,2154c2416,2417
< static int handle_else(ap_bucket *in, request_rec *r, const char *error,
< int *conditional_status, int *printing)
---
> static int handle_else(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
2156,2157c2419,2421
< char tag[MAX_STRING_LEN];
< apr_off_t offset = strlen("else ") + strlen(STARTING_SEQUENCE);
---
> char *tag = NULL;
> char *tag_val = NULL;
> ap_bucket *tmp_buck;
2159,2160c2423,2431
< if (!get_tag(r->pool, in, tag, sizeof(tag), 1, &offset)) {
< return 1;
---
> *inserted_head = NULL;
> tag_val = get_tag_and_value(ctx, &tag, 1);
> if ((tag != NULL) || (tag_val != NULL)) {
> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
> "else directive does not take tags in %s", r->filename);
> if (ctx->flags & FLAG_PRINTING) {
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
> }
> return -1;
2162c2433
< else if (!strcmp(tag, "done")) {
---
> else {
2164,2165c2435
< ap_rvputs(r, "**** else conditional_status=\"",
< *conditional_status ? "1" : "0", "\"\n", NULL);
---
> LOG_COND_STATUS(ctx, tmp_buck, head_ptr, *inserted_head, " else");
2167,2176c2437,2438
< *printing = !(*conditional_status);
< *conditional_status = 1;
< return 0;
< }
< else {
< ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
< "else directive does not take tags in %s",
< r->filename);
< if (*printing) {
< ap_rputs(error, r);
---
> if (ctx->flags & FLAG_COND_TRUE) {
> ctx->flags &= FLAG_CLEAR_PRINTING;
2178c2440,2443
< return -1;
---
> else {
> ctx->flags |= (FLAG_PRINTING | FLAG_COND_TRUE);
> }
> return 0;
2182,2183c2447,2448
< static int handle_endif(ap_bucket *in, request_rec *r, const char *error,
< int *conditional_status, int *printing)
---
> static int handle_endif(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
2185,2189c2450,2460
< char tag[MAX_STRING_LEN];
< apr_off_t offset = strlen("endif ") + strlen(STARTING_SEQUENCE);
<
< if (!get_tag(r->pool, in, tag, sizeof(tag), 1, &offset)) {
< return 1;
---
> char *tag = NULL;
> char *tag_val = NULL;
> ap_bucket *tmp_buck;
>
> *inserted_head = NULL;
> tag_val = get_tag_and_value(ctx, &tag, 1);
> if ((tag != NULL) || (tag_val != NULL)) {
> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
> "endif directive does not take tags in %s", r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
> return -1;
2191c2462
< else if (!strcmp(tag, "done")) {
---
> else {
2193,2194c2464
< ap_rvputs(r, "**** endif conditional_status=\"",
< *conditional_status ? "1" : "0", "\"\n", NULL);
---
> LOG_COND_STATUS(ctx, tmp_buck, head_ptr, *inserted_head, "endif");
2196,2197c2466
< *printing = 1;
< *conditional_status = 1;
---
> ctx->flags |= (FLAG_PRINTING | FLAG_COND_TRUE);
2200,2206d2468
< else {
< ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
< "endif directive does not take tags in %s",
< r->filename);
< ap_rputs(error, r);
< return -1;
< }
2209c2471,2472
< static int handle_set(ap_bucket *in, request_rec *r, const char *error)
---
> static int handle_set(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
2211c2474,2477
< char tag[MAX_STRING_LEN];
---
> char *tag = NULL;
> char *tag_val = NULL;
> char *var = NULL;
> ap_bucket *tmp_buck;
2213,2215d2478
< char *tag_val;
< char *var;
< apr_off_t offset = strlen("set ") + strlen(STARTING_SEQUENCE);
2217c2480
< var = (char *) NULL;
---
> *inserted_head = NULL;
2219,2222c2482,2483
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
< return 1;
< }
< else if (!strcmp(tag, "done")) {
---
> tag_val = get_tag_and_value(ctx, &tag, 1);
> if ((tag == NULL) && (tag_val == NULL)) {
2224a2486,2488
> else if (tag_val == NULL) {
> return 1;
> }
2233,2234c2497,2498
< ap_rputs(error, r);
< return -1;
---
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
> return (-1);
2237c2501,2502
< apr_table_setn(r->subprocess_env, var, apr_pstrdup(r->pool, parsed_string));
---
> apr_table_setn(r->subprocess_env, apr_pstrdup(r->pool, var),
> apr_pstrdup(r->pool, parsed_string));
2242c2507
< ap_rputs(error, r);
---
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
2248c2513,2514
< static int handle_printenv(ap_bucket *in, request_rec *r, const char *error)
---
> static int handle_printenv(include_ctx_t *ctx, request_rec *r, ap_bucket *head_ptr,
> ap_bucket **inserted_head)
2250,2255c2516,2526
< char tag[MAX_STRING_LEN];
< char *tag_val;
< apr_array_header_t *arr = apr_table_elts(r->subprocess_env);
< apr_table_entry_t *elts = (apr_table_entry_t *)arr->elts;
< int i;
< apr_off_t offset = strlen("printenv ") + strlen(STARTING_SEQUENCE);
---
> char *tag = NULL;
> char *tag_val = NULL;
> ap_bucket *tmp_buck;
>
> tag_val = get_tag_and_value(ctx, &tag, 1);
> if ((tag == NULL) && (tag_val == NULL)) {
> apr_array_header_t *arr = apr_table_elts(r->subprocess_env);
> apr_table_entry_t *elts = (apr_table_entry_t *)arr->elts;
> int i;
> char *key_text, *val_text;
> apr_ssize_t k_len, v_len;
2257,2260c2528
< if (!(tag_val = get_tag(r->pool, in, tag, sizeof(tag), 1, &offset))) {
< return 1;
< }
< else if (!strcmp(tag, "done")) {
---
> *inserted_head = NULL;
2262,2263c2530,2549
< ap_rvputs(r, ap_escape_html(r->pool, elts[i].key), "=",
< ap_escape_html(r->pool, elts[i].val), "\n", NULL);
---
> key_text = ap_escape_html(r->pool, elts[i].key);
> val_text = ap_escape_html(r->pool, elts[i].val);
> k_len = strlen(key_text);
> v_len = strlen(val_text);
>
> /* Key_text */
> tmp_buck = ap_bucket_create_transient(key_text, k_len);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
> /* = */
> tmp_buck = ap_bucket_create_immortal("=", 1);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> /* Value_text */
> tmp_buck = ap_bucket_create_transient(val_text, v_len);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> /* newline... */
> tmp_buck = ap_bucket_create_immortal("\n", 1);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
2269,2271c2555,2556
< "printenv directive does not take tags in %s",
< r->filename);
< ap_rputs(error, r);
---
> "printenv directive does not take tags in %s", r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_buck, head_ptr, *inserted_head);
2276a2562,2569
> #define SPLIT_AND_PASS_PRETAG_BUCKETS(brgd, cntxt) \
> if ((AP_BRIGADE_EMPTY(cntxt->ssi_tag_brigade)) && (cntxt->head_start_bucket != NULL)) { \
> ap_bucket_brigade *tag_plus; \
> \
> tag_plus = ap_brigade_split(brgd, cntxt->head_start_bucket); \
> ap_pass_brigade(f->next, brgd); \
> brgd = tag_plus; \
> } \
2278d2570
< /* -------------------------- The main function --------------------------- */
2280c2572
< /* This is a stub which parses a file descriptor. */
---
> /* -------------------------- The main function --------------------------- */
2282,2284d2573
< typedef struct include_ctx {
< ap_bucket_brigade *bb;
< } include_ctx;
2288,2294c2577,2578
< char directive[MAX_STRING_LEN], error[MAX_STRING_LEN];
< char timefmt[MAX_STRING_LEN];
< int noexec = ap_allow_options(r) & OPT_INCNOEXEC;
< int sizefmt;
< int if_nesting;
< int printing;
< int conditional_status;
---
> include_ctx_t *ctx = f->ctx;
> int is_buffer_allocated = 0;
2296,2297c2580
< ap_bucket *tagbuck, *dptr2;
< ap_bucket *endsec;
---
> ap_bucket *tmp_dptr, *ctx_end, *bb_end;
2299d2581
< include_ctx *ctx;
2302,2309d2583
< apr_cpystrn(error, DEFAULT_ERROR_MSG, sizeof(error));
< apr_cpystrn(timefmt, DEFAULT_TIME_FORMAT, sizeof(timefmt));
< sizefmt = SIZEFMT_KMG;
<
< /* Turn printing on */
< printing = conditional_status = 1;
< if_nesting = 0;
<
2320,2327c2594,2613
< if (!f->ctx) {
< f->ctx = ctx = apr_pcalloc(r->pool, sizeof(f->ctx));
< ctx->bb = ap_brigade_create(r->pool);
< }
< else {
< ctx = f->ctx;
< AP_BRIGADE_CONCAT(*bb, ctx->bb);
< }
---
> while (dptr != AP_BRIGADE_SENTINEL(*bb)) {
> /* State to check for the STARTING_SEQUENCE. */
> if ((ctx->state == PRE_HEAD) || (ctx->state == PARSE_HEAD)) {
> int do_cleanup = 0;
> apr_ssize_t cleanup_bytes = ctx->parse_pos;
>
> tmp_dptr = find_start_sequence(dptr, ctx, AP_BRIGADE_LAST(*bb), &do_cleanup);
>
> if ((do_cleanup) && (!AP_BRIGADE_EMPTY(ctx->ssi_tag_brigade))) {
> ap_bucket *tmp_bkt;
>
> tmp_bkt = ap_bucket_create_immortal(STARTING_SEQUENCE, cleanup_bytes);
> AP_BRIGADE_INSERT_HEAD(*bb, tmp_bkt);
>
> while (!AP_BRIGADE_EMPTY(ctx->ssi_tag_brigade)) {
> tmp_bkt = AP_BRIGADE_FIRST(ctx->ssi_tag_brigade);
> AP_BUCKET_REMOVE(tmp_bkt);
> ap_bucket_destroy(tmp_bkt);
> }
> }
2329,2335c2615,2625
< AP_BRIGADE_FOREACH(dptr, *bb) {
< if ((tagbuck = find_string(dptr, STARTING_SEQUENCE, AP_BRIGADE_LAST(*bb))) != NULL) {
< dptr2 = tagbuck;
< dptr = tagbuck;
< endsec = find_string(dptr2, ENDING_SEQUENCE, AP_BRIGADE_LAST(*bb));
< if (endsec == NULL) {
< ap_save_brigade(f, &ctx->bb, bb);
---
> /* If I am inside a conditional (if, elif, else) that is false
> * then I need to throw away anything contained in it.
> */
> if ((!(ctx->flags & FLAG_PRINTING)) && (tmp_dptr != NULL)) {
> while (dptr != tmp_dptr) {
> ap_bucket *free_bucket = dptr;
>
> dptr = AP_BUCKET_NEXT (dptr);
> AP_BUCKET_REMOVE(free_bucket);
> ap_bucket_destroy(free_bucket);
> }
2337,2338c2627,2664
<
< /* At this point, everything between tagbuck and endsec is an SSI
---
>
> /* Adjust the current bucket position based on what was found... */
> if ((tmp_dptr != NULL) && (ctx->state == PARSE_TAG)) {
> if (ctx->tag_start_bucket != NULL) {
> dptr = ctx->tag_start_bucket;
> }
> else {
> dptr = AP_BRIGADE_SENTINEL(*bb);
> }
> }
> else if (tmp_dptr == NULL) { /* There was no possible SSI tag in the */
> dptr = AP_BRIGADE_SENTINEL(*bb); /* remainder of this brigade... */
> }
> }
>
> /* State to check for the ENDING_SEQUENCE. */
> if (((ctx->state == PARSE_TAG) || (ctx->state == PARSE_TAIL)) &&
> (dptr != AP_BRIGADE_SENTINEL(*bb))) {
> tmp_dptr = find_end_sequence(dptr, ctx, AP_BRIGADE_LAST(*bb));
>
> if (tmp_dptr != NULL) {
> dptr = tmp_dptr; /* Adjust bucket pos... */
> }
> else {
> dptr = AP_BRIGADE_SENTINEL(*bb); /* remainder of this brigade... */
> }
> }
>
> /* State to processed the directive... */
> if (ctx->state == PARSED) {
> ap_bucket *content_head = NULL, *tmp_bkt;
> char tmp_buf[TMP_BUF_SIZE];
> char *directive_str = NULL;
> dir_token_id directive_token;
> int done = 0;
>
> /* At this point, everything between ctx->head_start_bucket and
> * ctx->tail_start_bucket is an SSI
2341c2667,2668
< if (get_directive(tagbuck, directive, sizeof(directive), r->pool)) {
---
> if (get_combined_directive (ctx, *bb, &is_buffer_allocated,
> tmp_buf, TMP_BUF_SIZE) != APR_SUCCESS) {
2343c2670
< "mod_include: error reading directive in %s",
---
> "mod_include: error copying directive in %s",
2345c2672,2702
< ap_rputs(error, r);
---
> CREATE_ERROR_BUCKET(ctx, tmp_bkt, dptr, content_head);
>
> /* DO CLEANUP HERE!!!!! */
> tmp_dptr = ctx->head_start_bucket;
> if (!AP_BRIGADE_EMPTY(ctx->ssi_tag_brigade)) {
> ctx_end = AP_BRIGADE_LAST(ctx->ssi_tag_brigade);
> }
> else {
> ctx_end = NULL;
> }
> bb_end = AP_BRIGADE_LAST(*bb);
>
> do {
> if (tmp_dptr == dptr) {
> done = 1;
> }
> else {
> ap_bucket *free_bucket;
>
> free_bucket = tmp_dptr;
> tmp_dptr = AP_BUCKET_NEXT (tmp_dptr);
>
> if ((ctx_end != NULL) && (AP_BUCKET_PREV(tmp_dptr) == ctx_end)) {
> tmp_dptr = AP_BRIGADE_FIRST(*bb);
> }
>
> AP_BUCKET_REMOVE(free_bucket);
> ap_bucket_destroy(free_bucket);
> }
> } while ((!done) && (AP_BUCKET_PREV(tmp_dptr) != bb_end));
>
2348,2353c2705,2715
< tag_and_after = ap_brigade_split(*bb, dptr);
< ap_pass_brigade(f->next, *bb); /* process what came before the tag */
< *bb = tag_and_after;
< if (!strcmp(directive, "if")) {
< if (!printing) {
< if_nesting++;
---
>
> /* Can't destroy the tag buckets until I'm done processing
> * because the combined_tag might just be pointing to
> * the contents of a single bucket!
> */
> directive_str = get_directive(ctx, &directive_token);
>
> switch (directive_token) {
> case TOK_IF:
> if (!ctx->flags & FLAG_PRINTING) {
> ctx->if_nesting_level++;
2356,2379c2718,2719
< ret = handle_if(tagbuck, r, error, &conditional_status,
< &printing);
< if_nesting = 0;
< }
< continue;
< }
< else if (!strcmp(directive, "else")) {
< if (!if_nesting) {
< ret = handle_else(tagbuck, r, error, &conditional_status,
< &printing);
< }
< continue;
< }
< else if (!strcmp(directive, "elif")) {
< if (!if_nesting) {
< ret = handle_elif(tagbuck, r, error, &conditional_status,
< &printing);
< }
< continue;
< }
< else if (!strcmp(directive, "endif")) {
< if (!if_nesting) {
< ret = handle_endif(tagbuck, r, error, &conditional_status,
< &printing);
---
> ret = handle_if(ctx, r, dptr, &content_head);
> ctx->if_nesting_level = 0;
2381,2382c2721,2725
< else {
< if_nesting--;
---
> break;
>
> case TOK_ELSE:
> if (!ctx->if_nesting_level) {
> ret = handle_else(ctx, r, dptr, &content_head);
2384,2396c2727,2737
< continue;
< }
< if (!printing) {
< continue;
< }
< if (!strcmp(directive, "exec")) {
< if (noexec) {
< ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
< "exec used but not allowed in %s",
< r->filename);
< if (printing) {
< ap_rputs(error, r);
< }
---
> break;
>
> case TOK_ELIF:
> if (!ctx->if_nesting_level) {
> ret = handle_elif(ctx, r, dptr, &content_head);
> }
> break;
>
> case TOK_ENDIF:
> if (!ctx->if_nesting_level) {
> ret = handle_endif(ctx, r, dptr, &content_head);
2399c2740
< ret = handle_exec(tagbuck, r, error, f->next);
---
> ctx->if_nesting_level--;
2400a2742,2744
> break;
> default:
> break;
2402,2406c2746,2801
< else if (!strcmp(directive, "config")) {
< ret = handle_config(tagbuck, r, error, timefmt, &sizefmt);
< }
< else if (!strcmp(directive, "set")) {
< ret = handle_set(tagbuck, r, error);
---
>
> if (ctx->flags & FLAG_PRINTING) {
> switch (directive_token) {
> case TOK_EXEC:
> if (ctx->flags & FLAG_NO_EXEC) {
> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
> "exec used but not allowed in %s", r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_bkt, dptr, content_head);
> }
> else {
> SPLIT_AND_PASS_PRETAG_BUCKETS(*bb, ctx);
> ret = handle_exec(ctx, r, f->next, dptr, &content_head);
> }
> break;
>
> case TOK_INCLUDE:
> SPLIT_AND_PASS_PRETAG_BUCKETS(*bb, ctx);
> ret = handle_include(ctx, r, f->next, dptr, &content_head);
> break;
>
> #ifdef USE_PERL_SSI
> case TOK_PERL:
> SPLIT_AND_PASS_PRETAG_BUCKETS(*bb, ctx);
> ret = handle_perl(ctx, r);
> break;
> #endif
> case TOK_SET: /* Might generate an error... */
> ret = handle_set(ctx, r, dptr, &content_head);
> break;
> case TOK_ECHO:
> ret = handle_echo(ctx, r, dptr, &content_head);
> break;
> case TOK_FSIZE:
> ret = handle_fsize(ctx, r, dptr, &content_head);
> break;
> case TOK_CONFIG: /* Might generate an error... */
> ret = handle_config(ctx, r, dptr, &content_head);
> break;
> case TOK_FLASTMOD:
> ret = handle_flastmod(ctx, r, dptr, &content_head);
> break;
> case TOK_PRINTENV:
> ret = handle_printenv(ctx, r, dptr, &content_head);
> break;
>
> default:
> if ((directive_token != TOK_IF) &&
> (directive_token != TOK_ELSE) &&
> (directive_token != TOK_ELIF) &&
> (directive_token != TOK_ENDIF)) {
> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
> "unknown directive \"%s\" in parsed doc %s",
> directive_str, r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_bkt, dptr, content_head);
> }
> }
2408,2409c2803,2809
< else if (!strcmp(directive, "include")) {
< ret = handle_include(tagbuck, r, f->next, error, noexec);
---
> else {
> if (directive_token == TOK_UNKNOWN) {
> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
> "unknown directive \"%s\" in parsed doc %s",
> directive_str, r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_bkt, dptr, content_head);
> }
2411,2412c2811,2831
< else if (!strcmp(directive, "echo")) {
< ret = handle_echo(tagbuck, r, error);
---
>
> /* This chunk of code starts at the first bucket in the chain
> * of tag buckets (assuming that by this point the bucket for
> * the STARTING_SEQUENCE has been split) and loops through to
> * the end of the tag buckets freeing them all.
> *
> * Remember that some part of this may have been set aside
> * into the ssi_tag_brigade and the remainder (possibly as
> * little as one byte) will be in the current brigade.
> *
> * The value of dptr should have been set during the
> * PARSE_TAIL state to the first bucket after the
> * ENDING_SEQUENCE.
> *
> * The value of content_head may have been set during processing
> * of the directive. If so, the content was inserted in front
> * of the dptr bucket. The inserted buckets should not be thrown
> * away here, but they should also not be parsed later.
> */
> if (content_head == NULL) {
> content_head = dptr;
2414,2415c2833,2835
< else if (!strcmp(directive, "fsize")) {
< ret = handle_fsize(tagbuck, r, error, sizefmt);
---
> tmp_dptr = ctx->head_start_bucket;
> if (!AP_BRIGADE_EMPTY(ctx->ssi_tag_brigade)) {
> ctx_end = AP_BRIGADE_LAST(ctx->ssi_tag_brigade);
2417,2418c2837,2838
< else if (!strcmp(directive, "flastmod")) {
< ret = handle_flastmod(tagbuck, r, error, timefmt);
---
> else {
> ctx_end = NULL;
2420,2421c2840,2864
< else if (!strcmp(directive, "printenv")) {
< ret = handle_printenv(tagbuck, r, error);
---
> bb_end = AP_BRIGADE_LAST(*bb);
>
> do {
> if (tmp_dptr == content_head) {
> done = 1;
> }
> else {
> ap_bucket *free_bucket;
>
> free_bucket = tmp_dptr;
> tmp_dptr = AP_BUCKET_NEXT (tmp_dptr);
>
> if ((ctx_end != NULL) && (AP_BUCKET_PREV(tmp_dptr) == ctx_end)) {
> tmp_dptr = AP_BRIGADE_FIRST(*bb);
> }
>
> AP_BUCKET_REMOVE(free_bucket);
> ap_bucket_destroy(free_bucket);
> }
> } while ((!done) && (AP_BUCKET_PREV(tmp_dptr) != bb_end));
>
> if (is_buffer_allocated) {
> free (ctx->combined_tag);
> ctx->combined_tag = NULL;
> is_buffer_allocated = 0;
2423,2425c2866,2868
< #ifdef USE_PERL_SSI
< else if (!strcmp(directive, "perl")) {
< ret = handle_perl(tagbuck, r, error);
---
> else if (ctx->combined_tag != NULL) {
> memset (ctx->combined_tag, '\0', ctx->tag_length);
> ctx->combined_tag = NULL;
2427,2434c2870,2886
< #endif
< else {
< ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
< "unknown directive \"%s\" "
< "in parsed doc %s",
< directive, r->filename);
< if (printing) {
< ap_rputs(error, r);
---
>
> /* Don't reset the flags or the nesting level!!! */
> ctx->parse_pos = 0;
> ctx->head_start_bucket = NULL;
> ctx->head_start_index = 0;
> ctx->tag_start_bucket = NULL;
> ctx->tag_start_index = 0;
> ctx->tail_start_bucket = NULL;
> ctx->tail_start_index = 0;
> ctx->curr_tag_pos = NULL;
> ctx->tag_length = 0;
>
> if (!AP_BRIGADE_EMPTY(ctx->ssi_tag_brigade)) {
> while (!AP_BRIGADE_EMPTY(ctx->ssi_tag_brigade)) {
> tmp_bkt = AP_BRIGADE_FIRST(ctx->ssi_tag_brigade);
> AP_BUCKET_REMOVE(tmp_bkt);
> ap_bucket_destroy(tmp_bkt);
2437,2438c2889,2890
< *bb = ap_brigade_split(tag_and_after, endsec);
< dptr = AP_BUCKET_PREV(endsec);
---
>
> ctx->state = PRE_HEAD;
2440,2441c2892,2943
< else {
< return;
---
> }
>
> /* If I am in the middle of parsing an SSI tag then I need to set aside
> * the pertinent trailing buckets and pass on the initial part of the
> * brigade. The pertinent parts of the next brigades will be added to
> * these set aside buckets to form the whole tag and will be processed
> * once the whole tag has been found.
> */
> if (ctx->state == PRE_HEAD) {
> /* Inside a false conditional (if, elif, else), so toss it all... */
> if (!(ctx->flags & FLAG_PRINTING)) {
> int done = 0;
> ap_bucket *last_bkt;
>
> last_bkt = AP_BRIGADE_LAST(*bb);
> do {
> ap_bucket *free_bucket = dptr;
>
> if (dptr == last_bkt) {
> done = 1;
> }
> else {
> dptr = AP_BUCKET_NEXT (dptr);
> AP_BUCKET_REMOVE(free_bucket);
> ap_bucket_destroy(free_bucket);
> }
> } while (!done);
> }
> else { /* Otherwise pass it along... */
> ap_pass_brigade(f->next, *bb); /* No SSI tags in this brigade... */
> }
> }
> else if (ctx->state == PARSED) { /* Invalid internal condition... */
> ap_bucket *content_head, *tmp_bkt;
> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
> "Invalid mod_include state during file %s", r->filename);
> CREATE_ERROR_BUCKET(ctx, tmp_bkt, AP_BRIGADE_FIRST(*bb), content_head);
> }
> else { /* Entire brigade is middle chunk of SSI tag... */
> if (!AP_BRIGADE_EMPTY(ctx->ssi_tag_brigade)) {
> AP_BRIGADE_CONCAT(ctx->ssi_tag_brigade, *bb);
> }
> else { /* End of brigade contains part of SSI tag... */
> if (ctx->head_start_index > 0) {
> ap_bucket_split(ctx->head_start_bucket, ctx->head_start_index);
> ctx->head_start_bucket = AP_BUCKET_NEXT(ctx->head_start_bucket);
> ctx->head_start_index = 0;
> }
> /* Set aside tag, pass pre-tag... */
> tag_and_after = ap_brigade_split(*bb, ctx->head_start_bucket);
> ap_save_brigade(f, &ctx->ssi_tag_brigade, &tag_and_after);
> ap_pass_brigade(f->next, *bb);
2492a2995
> include_ctx_t *ctx = f->ctx;
2496a3000,3004
> #ifdef FREDDY_KRUGER_VEGAMATIC_MODE
> ap_bucket_brigade *rest;
> ap_bucket *tmp_buck;
> #endif
>
2504a3013,3032
> if (!f->ctx) {
> f->ctx = ctx = apr_pcalloc(f->c->pool, sizeof(*ctx));
> if (ctx != NULL) {
> ctx->state = PRE_HEAD;
> ctx->flags = (FLAG_PRINTING | FLAG_COND_TRUE);
> if (ap_allow_options(r) & OPT_INCNOEXEC) {
> ctx->flags |= FLAG_NO_EXEC;
> }
> ctx->ssi_tag_brigade = ap_brigade_create(f->c->pool);
>
> apr_cpystrn(ctx->error_str, DEFAULT_ERROR_MSG, sizeof(ctx->error_str));
> apr_cpystrn(ctx->time_str, DEFAULT_TIME_FORMAT, sizeof(ctx->time_str));
> ctx->error_length = strlen(ctx->error_str);
> }
> else {
> ap_pass_brigade(f->next, b);
> return APR_ENOMEM;
> }
> }
>
2540a3069,3091
>
> #ifdef FREDDY_KRUGER_VEGAMATIC_MODE
> tmp_buck = AP_BRIGADE_FIRST(b);
> while (!AP_BUCKET_IS_EOS(tmp_buck)) {
> const char *buf;
> apr_ssize_t len;
>
> ap_bucket_read(tmp_buck, &buf, &len, 0);
>
> if (len > 1) {
> ap_bucket_split(tmp_buck, 1);
> }
> tmp_buck = AP_BUCKET_NEXT(tmp_buck);
> rest = ap_brigade_split(b, tmp_buck);
>
> send_parsed_content(&b, r, f);
>
> b = rest;
> }
> if (AP_BUCKET_IS_EOS(tmp_buck)) {
> send_parsed_content(&b, r, f);
> }
> #else
2542c3093
< ap_pass_brigade(f->next, b);
---
> #endif
-------------------------------------------------------------------------
-------------------------------------------------------------------------
/* ====================================================================
* The Apache Software License, Version 1.1
*
* Copyright (c) 2000 The Apache Software Foundation. All rights
* reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
*
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
*
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in
* the documentation and/or other materials provided with the
* distribution.
*
* 3. The end-user documentation included with the redistribution,
* if any, must include the following acknowledgment:
* "This product includes software developed by the
* Apache Software Foundation (http://www.apache.org/)."
* Alternately, this acknowledgment may appear in the software itself,
* if and wherever such third-party acknowledgments normally appear.
*
* 4. The names "Apache" and "Apache Software Foundation" must
* not be used to endorse or promote products derived from this
* software without prior written permission. For written
* permission, please contact apache@apache.org.
*
* 5. Products derived from this software may not be called "Apache",
* nor may "Apache" appear in their name, without prior written
* permission of the Apache Software Foundation.
*
* THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESSED OR IMPLIED
* WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE APACHE SOFTWARE FOUNDATION OR
* ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
* USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
* ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
* ====================================================================
*
* This software consists of voluntary contributions made by many
* individuals on behalf of the Apache Software Foundation. For more
* information on the Apache Software Foundation, please see
* <http://www.apache.org/>.
*
* Portions of this software are based upon public domain software
* originally written at the National Center for Supercomputing Applications,
* University of Illinois, Urbana-Champaign.
*/
#ifndef _MOD_INCLUDE_H
#define _MOD_INCLUDE_H 1
#define STARTING_SEQUENCE "<!--#"
#define ENDING_SEQUENCE "-->"
#define DEFAULT_ERROR_MSG "[an error occurred while processing this directive]"
#define DEFAULT_TIME_FORMAT "%A, %d-%b-%Y %H:%M:%S %Z"
#define SIZEFMT_BYTES 0
#define SIZEFMT_KMG 1
#define TMP_BUF_SIZE 1024
#ifdef CHARSET_EBCDIC
#define RAW_ASCII_CHAR(ch) apr_xlate_conv_byte(ap_hdrs_from_ascii, (unsigned char)ch)
#else /*CHARSET_EBCDIC*/
#define RAW_ASCII_CHAR(ch) (ch)
#endif /*CHARSET_EBCDIC*/
module AP_MODULE_DECLARE_DATA includes_module;
/* just need some arbitrary non-NULL pointer which can't also be a request_rec */
#define NESTED_INCLUDE_MAGIC (&includes_module)
/* TODO: changing directory should be handled by CreateProcess */
#define ap_chdir_file(x) do {} while(0)
/****************************************************************************
* Used to keep context information during parsing of a request for SSI tags.
* This is especially useful if the tag stretches across multiple buckets or
* brigades. This keeps track of which buckets need to be replaced with the
* content generated by the SSI tag.
*
* state: PRE_HEAD - State prior to finding the first character of the
* STARTING_SEQUENCE. Next state is PARSE_HEAD.
* PARSE_HEAD - State entered once the first character of the
* STARTING_SEQUENCE is found and exited when the
* the full STARTING_SEQUENCE has been matched or
* a match failure occurs. Next state is PRE_HEAD
* or PARSE_TAG.
* PARSE_TAG - State entered once the STARTING sequence has been
* matched. It is exited when the first character in
* ENDING_SEQUENCE is found. Next state is PARSE_TAIL.
* PARSE_TAIL - State entered from PARSE_TAG state when the first
* character in ENDING_SEQUENCE is encountered. This
* state is exited when the ENDING_SEQUENCE has been
* completely matched, or when a match failure occurs.
* Next state is PARSE_TAG or PARSED.
* PARSED - State entered from PARSE_TAIL once the complete
* ENDING_SEQUENCE has been matched. The SSI tag is
* processed and the SSI buckets are replaced with the
* SSI content during this state.
* parse_pos: Current matched position within the STARTING_SEQUENCE or
* ENDING_SEQUENCE during the PARSE_HEAD and PARSE_TAIL states.
* This is especially useful when the sequence spans brigades.
* X_start_bucket: These point to the buckets containing the first character
* of the STARTING_SEQUENCE, the first non-whitespace
* character of the tag, and the first character in the
* ENDING_SEQUENCE (head_, tag_, and tail_ respectively).
* The buckets are kept intact until the PARSED state is
* reached, at which time the tag is consolidated and the
* buckets are released. The buckets that these point to
* have all been set aside in the ssi_tag_brigade (along
* with all of the intervening buckets).
* X_start_index: The index points within the specified bucket contents
* where the first character of the STARTING_SEQUENCE,
* the first non-whitespace character of the tag, and the
* first character in the ENDING_SEQUENCE can be found
* (head_, tag_, and tail_ respectively).
* combined_tag: Once the PARSED state is reached the tag is collected from
* the bucket(s) in the ssi_tag_brigade into this contiguous
* buffer. The buckets in the ssi_tag_brigade are released
* and the tag is processed.
* curr_tag_pos: Ptr to the combined_tag buffer indicating the current
* parse position.
* tag_length: The number of bytes in the actual tag (excluding the
* STARTING_SEQUENCE, leading and trailing whitespace,
* and ENDING_SEQUENCE). This length is computed as the
* buckets are parsed and set aside during the PARSE_TAG state.
* ssi_tag_brigade: The temporary brigade used by this filter to set aside
* the buckets containing parts of the ssi tag and headers.
*/
typedef enum {PRE_HEAD, PARSE_HEAD, PARSE_TAG, PARSE_TAIL, PARSED} states;
typedef struct include_filter_ctx {
states state;
long flags; /* See the FLAG_XXXXX definitions. */
int if_nesting_level;
apr_ssize_t parse_pos;
ap_bucket *head_start_bucket;
apr_ssize_t head_start_index;
ap_bucket *tag_start_bucket;
apr_ssize_t tag_start_index;
ap_bucket *tail_start_bucket;
apr_ssize_t tail_start_index;
char *combined_tag;
char *curr_tag_pos;
apr_ssize_t tag_length;
apr_ssize_t error_length;
char error_str[MAX_STRING_LEN];
char time_str[MAX_STRING_LEN];
ap_bucket_brigade *ssi_tag_brigade;
} include_ctx_t;
/* These flags are used to set flag bits. */
#define FLAG_PRINTING 0x00000001 /* Printing conditional lines. */
#define FLAG_COND_TRUE 0x00000002 /* Conditional eval'd to true. */
#define FLAG_SIZE_IN_BYTES 0x00000004 /* Sizes displayed in bytes. */
#define FLAG_NO_EXEC 0x00000008 /* No Exec in current context. */
/* These flags are used to clear flag bits. */
#define FLAG_SIZE_ABBREV 0xFFFFFFFB /* Reset SIZE_IN_BYTES bit. */
#define FLAG_CLEAR_PRINT_COND 0xFFFFFFFC /* Reset PRINTING and COND_TRUE*/
#define FLAG_CLEAR_PRINTING 0xFFFFFFFE /* Reset just PRINTING bit. */
typedef enum {TOK_UNKNOWN, TOK_IF, TOK_SET, TOK_ECHO, TOK_ELIF, TOK_ELSE,
TOK_EXEC, TOK_PERL, TOK_ENDIF, TOK_FSIZE, TOK_CONFIG,
TOK_INCLUDE, TOK_FLASTMOD, TOK_PRINTENV} dir_token_id;
#define CREATE_ERROR_BUCKET(cntx, t_buck, h_ptr, ins_head) \
t_buck = ap_bucket_create_transient(cntx->error_str, \
ctx->error_length); \
AP_BUCKET_INSERT_BEFORE(h_ptr, t_buck); \
\
if (ins_head == NULL) { \
ins_head = t_buck; \
}
#endif /* _MOD_INCLUDE_H */
Re: [Patch]: mod_include.c port to 2.0 buckets (includes
newmod_include.h)
Posted by rb...@covalent.net.
On Tue, 21 Nov 2000, Paul J. Reder wrote:
> rbb@covalent.net wrote:
> > It fails in almost the exact some ways and places that the
> > current mod_include fails, except it is harder to debug and maintain.
>
> Which failures are you refering to. I am only aware of the content
> length failure and the potential loop due to ap_r* calls in the error
> path of parse_expr. Are there others that you have seen?
>
> I ask for my debugging purposes only, not to advocate putting it in
> CVS yet (which I don't). Give me a chance to implement your suggestions,
> and don't dis' the code just because it is different. As Greg points
> out, the added complexity was required to deal with the more complex
> situation that a filter faces in dealing with brigades of buckets.
I have already removed the content-length from my development tree. It
doesn't fix any of the problems with trying to serve the FAQ. There is a
problem with the tag parsing someplace, because when I request the FAQ
with netscape, I can alternate between two pages, the first ends with
<!C-- If adding a relative link to another
The second begins with
-->
Let me repeat, this is not a bug with content-length that I am aware
of. In my tree, the content-length is not added by anything other than
the c-l filter.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes
newmod_include.h)
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
rbb@covalent.net wrote:
> It fails in almost the exact some ways and places that the
> current mod_include fails, except it is harder to debug and maintain.
Which failures are you refering to. I am only aware of the content
length failure and the potential loop due to ap_r* calls in the error
path of parse_expr. Are there others that you have seen?
I ask for my debugging purposes only, not to advocate putting it in
CVS yet (which I don't). Give me a chance to implement your suggestions,
and don't dis' the code just because it is different. As Greg points
out, the added complexity was required to deal with the more complex
situation that a filter faces in dealing with brigades of buckets.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new mod_include.h)
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 21, 2000 at 12:15:09PM -0800, rbb@covalent.net wrote:
>
> > apr_isspace() is trustable. We use it in ap_getword_white(), meaning we use
> > it all over :-)
>
> In that case, wouldn't this be even safer:
>
>
> dest = ap_getword_white(c)
That makes a copy of the string. We already have enough copies around :-)
> if (strncasecmp(c, "if", strlen("if")B) ...
You certainly wouldn't need to use strncasecmp() if you've done a getword.
There isn't any advantage to that over a regular strcasecmp().
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new mod_include.h)
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 21, 2000 at 07:42:16PM -0500, Paul J. Reder wrote:
> Greg Stein wrote:
> > On Tue, Nov 21, 2000 at 06:27:49AM -0800, rbb@covalent.net wrote:
>...
> > > In get_directive:
>...
> First, the isspace and tolower stuff was just a port of how it has been
> doing things since 1.3. If its broken the world sure has been fooled.
>
> Second, the strcmps are all going away when I get the directive hash table
> implemented. I still need to get the delimited directive, but no string
> compares get done.
>
> Third, the directive token location could be optimized into the initial
> pass through looking for the end of the tag. In the function find_end_sequence
> I already identify the beginning of the directive, I could just as easily add
> a state to find the end of the directive. No additional passes through the bytes
> since I have to go through them once any way. But this all seems like a lot
> of fuss for a few bytes per directive.
Agreed on all accounts.
I totally support punting the hash stuff until we actually have a working
version! :-) ... and yes: optimizing that directive stuff is a bit too
early. There are other issues to concentrate on... such as, "does it work?"
:-)
Good work, Paul. I look forward to the next revision.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new
mod_include.h)
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
Greg Stein wrote:
>
> On Tue, Nov 21, 2000 at 06:27:49AM -0800, rbb@covalent.net wrote:
> >
> > Something has been bothering me ever since I reviewed this code. I missed
> > something. Here is another comment on this code.
> >
> >
> > In get_directive:
> >
> > > dest = c;
> > > /* now get directive */
> > > while ((*c != '\0') && (!apr_isspace(*c))) {
> > > *c++ = apr_tolower(*c);
> > > len++;
> > > }
> > >
> > > *c++ = '\0';
> > > ctx->curr_tag_pos = c;
> > >
> > > *fnd_token = TOK_UNKNOWN;
> > > switch (len) {
> > > case 2: if (!strcmp(dest, "if")) *fnd_token = TOK_IF;
> > > break;
> > > ...
> >
> > The ... are because there are a lot more of those strcmps. Why? This
> > whole section of code can be made cleaner and more bug free (I don't trust
> > apr_isspace very much)
>
> apr_isspace() is trustable. We use it in ap_getword_white(), meaning we use
> it all over :-)
First, the isspace and tolower stuff was just a port of how it has been
doing things since 1.3. If its broken the world sure has been fooled.
Second, the strcmps are all going away when I get the directive hash table
implemented. I still need to get the delimited directive, but no string
compares get done.
Third, the directive token location could be optimized into the initial
pass through looking for the end of the tag. In the function find_end_sequence
I already identify the beginning of the directive, I could just as easily add
a state to find the end of the directive. No additional passes through the bytes
since I have to go through them once any way. But this all seems like a lot
of fuss for a few bytes per directive.
> That said: the above code possibly has a bug:
>
> > > *c++ = apr_tolower(*c);
>
> I don't believe there is an "execution point" defined in the above statement
> to ensure that the c++ occurs *after* the *c. The loop should probably be
> rewritten to look something like:
>
> > > while (((ch = *c) != '\0') && (!apr_isspace(ch))) {
> > > *c++ = apr_tolower(ch);
> > > len++;
> > > }
Good catch. I'm not sure whether it is absolutely necessary or not, but I will
fix it to make it clearer if nothing else.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new
mod_include.h)
Posted by rb...@covalent.net.
> apr_isspace() is trustable. We use it in ap_getword_white(), meaning we use
> it all over :-)
In that case, wouldn't this be even safer:
dest = ap_getword_white(c)
if (strncasecmp(c, "if", strlen("if")B) ...
> Nope. That won't work. If you use strncasecmp(), then you could end up
> matching against "<!--#if-ryan-is-a-dork" :-)
I figured that out on my way after I had sent and forgotten about the
mail. :-(
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new mod_include.h)
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Nov 21, 2000 at 06:27:49AM -0800, rbb@covalent.net wrote:
>
> Something has been bothering me ever since I reviewed this code. I missed
> something. Here is another comment on this code.
>
>
> In get_directive:
>
> > dest = c;
> > /* now get directive */
> > while ((*c != '\0') && (!apr_isspace(*c))) {
> > *c++ = apr_tolower(*c);
> > len++;
> > }
> >
> > *c++ = '\0';
> > ctx->curr_tag_pos = c;
> >
> > *fnd_token = TOK_UNKNOWN;
> > switch (len) {
> > case 2: if (!strcmp(dest, "if")) *fnd_token = TOK_IF;
> > break;
> > ...
>
> The ... are because there are a lot more of those strcmps. Why? This
> whole section of code can be made cleaner and more bug free (I don't trust
> apr_isspace very much)
apr_isspace() is trustable. We use it in ap_getword_white(), meaning we use
it all over :-)
> by removing the whole top section of this function,
> and just using strncasecmp(dest, "if", strlen("if")), plus this keeps us
> from traversing the same segment of the string twice.
Nope. That won't work. If you use strncasecmp(), then you could end up
matching against "<!--#if-ryan-is-a-dork" :-)
There are certainly ways to optimize the code, but strncasecmp() isn't going
to be one of them.
That said: the above code possibly has a bug:
> > *c++ = apr_tolower(*c);
I don't believe there is an "execution point" defined in the above statement
to ensure that the c++ occurs *after* the *c. The loop should probably be
rewritten to look something like:
> > while (((ch = *c) != '\0') && (!apr_isspace(ch))) {
> > *c++ = apr_tolower(ch);
> > len++;
> > }
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new
mod_include.h)
Posted by rb...@covalent.net.
Something has been bothering me ever since I reviewed this code. I missed
something. Here is another comment on this code.
In get_directive:
> dest = c;
> /* now get directive */
> while ((*c != '\0') && (!apr_isspace(*c))) {
> *c++ = apr_tolower(*c);
> len++;
> }
>
> *c++ = '\0';
> ctx->curr_tag_pos = c;
>
> *fnd_token = TOK_UNKNOWN;
> switch (len) {
> case 2: if (!strcmp(dest, "if")) *fnd_token = TOK_IF;
> break;
> ...
The ... are because there are a lot more of those strcmps. Why? This
whole section of code can be made cleaner and more bug free (I don't trust
apr_isspace very much) by removing the whole top section of this function,
and just using strncasecmp(dest, "if", strlen("if")), plus this keeps us
from traversing the same segment of the string twice.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new
mod_include.h)
Posted by rb...@covalent.net.
> >...
> > You have ap_r* functions in the main-line code. This is not allowed. #1,
> > a handler that uses buckets cannot also use ap_r*. #2 and more important,
> > NO filter can use ap_r* functions, ever.
>
> A handler *can* mix buckets and ap_r*. There is NO problem with that.
>
> Ryan's point #2 is correct, though. If a filter tries to use them, it could
> easily lead to an infinite loop (since ap_r* calls into the top of the
> filter chain, which eventually calls the filter, which calls ap_r* ...)
There will definately be a problem with mixing buckets and ap_r*. People
have been talking for a while about having the ap_r* functions buffer the
data. It is an undefined operation IMHO.
> > Think of it this way. Right now, this code is not easily extensible, to
> > make it extensible is easy however. Instead of hard-coding the directives
> > that are understood, take a modular approach. Use a hash table to store
> > the directives that are understood, and just pass the processing off to
> > those functions. This allows another module to register a new SSI
> > directive easily. In order for this to work, the handle_* function needs
> > to have complete discretion over how the brigade is handled. Again, this
> > would simplify code, making this module easier to debug.
>
> Note that there are two steps to getting mod_include to work. The initial
> port, then the introduction of new features.
>
> I totally support Paul's sticking with hard-coding the directives. We can
> modularize this portion later.
I would support it if it worked. It doesn't, and the code has become VERY
complex.
> > I have started to try to debug this module, but I am giving up now. I am
> > unlikely to touch this version of the module again, because it is just too
> > much. I'm going back to the original version. If somebody else wants to
> > debug the new one, feel free.
>
> I would NOT recommend bailing on this one. Anything that you try is going to
> be similar, so let's do a round on two on this version before you make a
> call like this. You've provided a lot of feedback on this rev. Let Paul
> integrate that (and/or answer questions about "why" things were in there).
> When the new mod_include arrives, then we can give another review.
I didn't say everybody should give up on it. I said I was giving up on it
currently. It fails in almost the exact some ways and places that the
current mod_include fails, except it is harder to debug and maintain.
> For myself, I am skipping a review of this revision since Ryan has provided
> so much useful feedback. Paul: when you issue a second revision
> (incorporating Ryan's suggestions), then I will review that. Since our
> current mod_include doesn't work, I would probably even recommend just
> checking the darn thing in (CTR), then iterating from there.
-1. The two mod_includes fail in the same way, except the current one is
much more maintainable. Until this patch is cleaned up a lot, I don't
want to have to deal with it in CVS. I am much happier fixing the current
mod_include, and targeting the full re-write for a later beta or even
after the actual release.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new mod_include.h)
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 20, 2000 at 05:14:58PM -0800, rbb@covalent.net wrote:
>...
> get_directive should use a hash table to store the tags. The key is the
> directive, the value is the function to call when that directive is
> encountered. (This also allows modules to easily extend mod_include's
> capabilities to handle more directives, allowing us to remove
> USE_PERL_SSI, because that becomes part of mod_perl.)
But that would be a run-time data structure. I'm not sure that we want to
set up runtime structures just to deal with parsing.
That said... I can also see the nice extension story for it.
>...
> You can't do this with any transient buckets. The point behind transient
> buckets is that they are stack data, but in order for them to work, you
> have to call the set-aside function on those buckets.
You can do one of two things with transient buckets:
1) create them, insert them in a brigade, and call ap_pass_brigade()
[ this is their primary purpose ]
2) create them and set them aside, as Ryan mentions. Of course, this is
silly, and the bucket should just be created as heap or pool bucket.
Transient buckets are *very* nice, but they need to be used in the right
situations.
>...
> You have ap_r* functions in the main-line code. This is not allowed. #1,
> a handler that uses buckets cannot also use ap_r*. #2 and more important,
> NO filter can use ap_r* functions, ever.
A handler *can* mix buckets and ap_r*. There is NO problem with that.
Ryan's point #2 is correct, though. If a filter tries to use them, it could
easily lead to an infinite loop (since ap_r* calls into the top of the
filter chain, which eventually calls the filter, which calls ap_r* ...)
>...
> Think of it this way. Right now, this code is not easily extensible, to
> make it extensible is easy however. Instead of hard-coding the directives
> that are understood, take a modular approach. Use a hash table to store
> the directives that are understood, and just pass the processing off to
> those functions. This allows another module to register a new SSI
> directive easily. In order for this to work, the handle_* function needs
> to have complete discretion over how the brigade is handled. Again, this
> would simplify code, making this module easier to debug.
Note that there are two steps to getting mod_include to work. The initial
port, then the introduction of new features.
I totally support Paul's sticking with hard-coding the directives. We can
modularize this portion later.
>...
> In general, instead of the re-write making mod_include easier to deal
> with, it has become a bigger monster, doubling in size and adding another
> 600 lines of code (not counting the include file). I don't mind more
> lines of code, I do mind when those lines of code are almost impossible to
> wade through.
This is just the first pass. It is quite likely that with your feedback,
plus some other, that Paul can clean it up some.
Note that we are completely changing how mod_include works. Before, it was a
handler, and now it is a filter. That is a drastic change, so I would expect
it to look quite a bit different. It also has a lot more accounting to deal
with, considering the brigade problem (with tags split across buckets and
brigades). Dealing with that can add some overhead.
600 more lines? Doesn't surprise me. But don't forget this was the initial
patch.
> I have started to try to debug this module, but I am giving up now. I am
> unlikely to touch this version of the module again, because it is just too
> much. I'm going back to the original version. If somebody else wants to
> debug the new one, feel free.
I would NOT recommend bailing on this one. Anything that you try is going to
be similar, so let's do a round on two on this version before you make a
call like this. You've provided a lot of feedback on this rev. Let Paul
integrate that (and/or answer questions about "why" things were in there).
When the new mod_include arrives, then we can give another review.
Doing a review of a second revision from Paul is going to consume a LOT less
of your time than trying to duplicate his work and redevelop a filter-based
mod_include.
For myself, I am skipping a review of this revision since Ryan has provided
so much useful feedback. Paul: when you issue a second revision
(incorporating Ryan's suggestions), then I will review that. Since our
current mod_include doesn't work, I would probably even recommend just
checking the darn thing in (CTR), then iterating from there.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: computing C-L (was: Re: [Patch]: mod_include.c ...)
Posted by Jeff Trawick <tr...@bellsouth.net>.
"Paul J. Reder" <re...@raleigh.ibm.com> writes:
> rbb@covalent.net wrote:
> > Just always unset it. The core always figures out the c-l if it needs to
> > and can. If it can't or doesn't need to, the core does the right thing.
>
> I can hunt through and figure it out, but it might save time to give
> me a quick clue as to how to "Just always unset it." Like which function
> to call to mess with header fields.
apr_table_unset(f->r->headers_out, "Content-Length");
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: computing C-L (was: Re: [Patch]: mod_include.c ...)
Posted by rb...@covalent.net.
On Tue, 21 Nov 2000, Paul J. Reder wrote:
> rbb@covalent.net wrote:
> > Just always unset it. The core always figures out the c-l if it needs to
> > and can. If it can't or doesn't need to, the core does the right thing.
>
> I can hunt through and figure it out, but it might save time to give
> me a quick clue as to how to "Just always unset it." Like which function
> to call to mess with header fields.
call
apr_table_unset(r->headers_out, "Content-Length");
should work.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: computing C-L (was: Re: [Patch]: mod_include.c ...)
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
rbb@covalent.net wrote:
> Just always unset it. The core always figures out the c-l if it needs to
> and can. If it can't or doesn't need to, the core does the right thing.
I can hunt through and figure it out, but it might save time to give
me a quick clue as to how to "Just always unset it." Like which function
to call to mess with header fields.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: computing C-L (was: Re: [Patch]: mod_include.c ...)
Posted by rb...@covalent.net.
Just always unset it. The core always figures out the c-l if it needs to
and can. If it can't or doesn't need to, the core does the right thing.
Ryan
On Tue, 21 Nov 2000, Paul J. Reder wrote:
> I need to know what mod_include should do in this case. I can certainly unset
> the field, but I would have to unset it before I know if it needs to change
> (I think).
>
> I certainly don't think I should try to compute the length because
> a single request may have several SSIs in it requiring the field to change as
> I encounter each one. And to make matters worse, some of the content is
> generated inside sub requests.
>
> Any help would be greatly appreciated. No version of mod_include will be able
> to work properly until this is resolved.
>
> --
> Paul J. Reder
> -----------------------------------------------------------
> "The strength of the Constitution lies entirely in the determination of each
> citizen to defend it. Only if every single citizen feels duty bound to do
> his share in this defense are the constitutional rights secure."
> -- Albert Einstein
>
>
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: computing C-L (was: Re: [Patch]: mod_include.c ...)
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
I need to know what mod_include should do in this case. I can certainly unset
the field, but I would have to unset it before I know if it needs to change
(I think).
I certainly don't think I should try to compute the length because
a single request may have several SSIs in it requiring the field to change as
I encounter each one. And to make matters worse, some of the content is
generated inside sub requests.
Any help would be greatly appreciated. No version of mod_include will be able
to work properly until this is resolved.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: computing C-L (was: Re: [Patch]: mod_include.c ...)
Posted by rb...@covalent.net.
> > > If a filter unsets the Content-Length header field, magic should
> > > happen (i.e., the core code should chunk or compute content length or
> > > let it stream as appropriate). The core support for this has been
> > > working in the past. A couple of quick tests with mod_autoindex
> > > (HTTP/1.0 and HTTP/1.1) indicates that the right stuff is happening.
> >
> > As things stand right now, if the c-l is set by the handler, the c-l
> > filter ignores it and re-computes if possible.
>
> But what if the handler got it right, and there are no intervening filters?
> Why should the C-L filter buffer up the response to recompute the thing?
The c-l filter should almost never buffer the whole response. It should
only ever buffer everything if it is a 1.1 request and it can't chunk for
some reason.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
computing C-L (was: Re: [Patch]: mod_include.c ...)
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 20, 2000 at 12:31:28PM -0800, rbb@covalent.net wrote:
>
> > A filter which expects to change the content length should unset the
> > Content-Length header field.
>
> No. That is making the filter do too much. There is no reason to ever
> set the content-length in any module. The core can do it much
> better. Allow me to explain a bit. If the handler sets the C-L, and
> mod_include unsets it, and the re-sets it to the new value, and then
> mod_gzip unsets it and re-sets it to the correct value, then we have just
> computed the thing three times, but only the last one was valid.
>
> All handlers and filters should punt the c-l computation to the c-l
> filter. If that filter can compute the c-l, then it does, once. If it
> can't, then it just passes the data on. It makes no sense for filters to
> be trying to set this, because it makes it harder to write
> filters/handlers, and the information is almost never going to be used.
We should not always punt to the C-L filter. That filter must have the whole
output brigade available to compute the length. It is entirely possible that
the handler knows the proper length and can set the value. It is the
presence of a filter that changes the length, which is the problem.
I'm perfectly okay with a filter unsetting the C-L because it is changing
the length.
Consider it from the semantic standpoint: the handler says, "here is the
content, and it is <this> long." But there is this sneaky filter hanging out
in there (and the handler should *not* know that), and the filter says
"well, I'm gonna change the length."
Whether the filter knows the final length or not is beside the point. The
handler should be able to set it, and the filter should unset it.
> > If a filter unsets the Content-Length header field, magic should
> > happen (i.e., the core code should chunk or compute content length or
> > let it stream as appropriate). The core support for this has been
> > working in the past. A couple of quick tests with mod_autoindex
> > (HTTP/1.0 and HTTP/1.1) indicates that the right stuff is happening.
>
> As things stand right now, if the c-l is set by the handler, the c-l
> filter ignores it and re-computes if possible.
But what if the handler got it right, and there are no intervening filters?
Why should the C-L filter buffer up the response to recompute the thing?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [Patch]: mod_include.c port to 2.0 buckets (includes newmod_include.h)
Posted by Jeff Trawick <tr...@bellsouth.net>.
rbb@covalent.net writes:
> > A filter which expects to change the content length should unset the
> > Content-Length header field.
>
> No. That is making the filter do too much. There is no reason to ever
> set the content-length in any module.
Oops... I didn't realize that we started ignored content length fields
set by the handler (or a filter).
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [Patch]: mod_include.c port to 2.0 buckets (includes newmod_include.h)
Posted by rb...@covalent.net.
> A filter which expects to change the content length should unset the
> Content-Length header field.
No. That is making the filter do too much. There is no reason to ever
set the content-length in any module. The core can do it much
better. Allow me to explain a bit. If the handler sets the C-L, and
mod_include unsets it, and the re-sets it to the new value, and then
mod_gzip unsets it and re-sets it to the correct value, then we have just
computed the thing three times, but only the last one was valid.
All handlers and filters should punt the c-l computation to the c-l
filter. If that filter can compute the c-l, then it does, once. If it
can't, then it just passes the data on. It makes no sense for filters to
be trying to set this, because it makes it harder to write
filters/handlers, and the information is almost never going to be used.
> If a filter unsets the Content-Length header field, magic should
> happen (i.e., the core code should chunk or compute content length or
> let it stream as appropriate). The core support for this has been
> working in the past. A couple of quick tests with mod_autoindex
> (HTTP/1.0 and HTTP/1.1) indicates that the right stuff is happening.
As things stand right now, if the c-l is set by the handler, the c-l
filter ignores it and re-computes if possible.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes newmod_include.h)
Posted by Jeff Trawick <tr...@bellsouth.net>.
"Bill Stoddard" <bi...@wstoddard.com> writes:
> There is definitely a content length problem. parsed content should not have a
> content length header added, yet the default handler is adding one. I suspect
> it should be removed and chunked encoding set somewhere in the path, I am just
> not yet sure where this should happen.
>
> Bill
A filter which expects to change the content length should unset the
Content-Length header field.
If a filter unsets the Content-Length header field, magic should
happen (i.e., the core code should chunk or compute content length or
let it stream as appropriate). The core support for this has been
working in the past. A couple of quick tests with mod_autoindex
(HTTP/1.0 and HTTP/1.1) indicates that the right stuff is happening.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [Patch]: mod_include.c port to 2.0 buckets (includes newmod_include.h)
Posted by rb...@covalent.net.
On Mon, 20 Nov 2000, Bill Stoddard wrote:
> There is definitely a content length problem. parsed content should not have a
> content length header added, yet the default handler is adding one. I suspect
> it should be removed and chunked encoding set somewhere in the path, I am just
> not yet sure where this should happen.
We are setting content-length for everything, and that is definately
wrong. But, there is no reason that parsed content can't have a
content-length, it just has to be the right c-l. :-)
This patch has many more issues than just that though. I will have the
time to outline most of them after the alpha.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes newmod_include.h)
Posted by Bill Stoddard <bi...@wstoddard.com>.
There is definitely a content length problem. parsed content should not have a
content length header added, yet the default handler is adding one. I suspect
it should be removed and chunked encoding set somewhere in the path, I am just
not yet sure where this should happen.
Bill
> This was the one odd behavior that I commented on in my initial note. I
wasn't
> able to track anything down in the mod_include code (not to imply that there
isn't
> something there - just that I couldn't find it). The browser doesn't seem
> to notice a problem (no errors reported anywhere). I thought it might be a
> content-length problem or something related to buffering or coalescing.
>
> The stopping point never seemed to relate to any bucket/brigade boundary as
seen
> by mod_include.
>
> All of the directives test out individually and in various groupings. The
problem
> shows up only randomly on larger files.
>
> There is no buffering done in mod_include that might be impacting this.
>
> It occurs to me that mod_include does not set a content length, but it also
does
> not unset a previous value. Should mod_include be clearing or correcting
this
> field?
>
> --
> Paul J. Reder
> -----------------------------------------------------------
> "The strength of the Constitution lies entirely in the determination of each
> citizen to defend it. Only if every single citizen feels duty bound to do
> his share in this defense are the constitutional rights secure."
> -- Albert Einstein
>
Re: [Patch]: mod_include.c port to 2.0 buckets (includes
newmod_include.h)
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
This was the one odd behavior that I commented on in my initial note. I wasn't
able to track anything down in the mod_include code (not to imply that there isn't
something there - just that I couldn't find it). The browser doesn't seem
to notice a problem (no errors reported anywhere). I thought it might be a
content-length problem or something related to buffering or coalescing.
The stopping point never seemed to relate to any bucket/brigade boundary as seen
by mod_include.
All of the directives test out individually and in various groupings. The problem
shows up only randomly on larger files.
There is no buffering done in mod_include that might be impacting this.
It occurs to me that mod_include does not set a content length, but it also does
not unset a previous value. Should mod_include be clearing or correcting this
field?
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new
mod_include.h)
Posted by rb...@covalent.net.
-1 for including this patch as is. A very quick test shows that it
doesn't serve our manual pages reliably. Also, a quick perusal of the
code shows some real problems. I am going to be spending the next few
hours getting the alpha ready. When I am done, I will try to do a much
more in-depth ananlysis of this patch.
Ryan
On Mon, 20 Nov 2000 rbb@covalent.net wrote:
> On Mon, 20 Nov 2000, Paul J. Reder wrote:
>
> > Finally, The promised port/rewrite of mod_include. Sorry it took so long.
> >
> > This is a CVS diff from locus. It includes the new mod_include.h file
> > after the mod_include.c diff listing. Sorry it is so long, but it was
> > a rewrite ;)
>
> I have just put this on my machine and tried to serve the FAQ. I am
> getting some very strange results. If I request the page multiple times,
> I get different results. The first time, I get the top of the page,
> ending in
>
> "<!C-- If adding a relative link to another part of the"
>
> The second time I get everything but the top portion of the page, starting
> with
>
> "-->"
>
> These two results pretty consistently alternate although sometimes I have
> to make multiple requests before they will switch.
>
> I am looking into the problem, but I am hesitant to commit this patch if
> it can't serve the FAQ. Do people disagree with that?
>
> Ryan
>
> _______________________________________________________________________________
> Ryan Bloom rbb@apache.org
> 406 29th St.
> San Francisco, CA 94131
> -------------------------------------------------------------------------------
>
>
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new
mod_include.h)
Posted by rb...@covalent.net.
On Mon, 20 Nov 2000, Paul J. Reder wrote:
> Finally, The promised port/rewrite of mod_include. Sorry it took so long.
>
> This is a CVS diff from locus. It includes the new mod_include.h file
> after the mod_include.c diff listing. Sorry it is so long, but it was
> a rewrite ;)
I have just put this on my machine and tried to serve the FAQ. I am
getting some very strange results. If I request the page multiple times,
I get different results. The first time, I get the top of the page,
ending in
"<!C-- If adding a relative link to another part of the"
The second time I get everything but the top portion of the page, starting
with
"-->"
These two results pretty consistently alternate although sometimes I have
to make multiple requests before they will switch.
I am looking into the problem, but I am hesitant to commit this patch if
it can't serve the FAQ. Do people disagree with that?
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes
newmod_include.h)
Posted by rb...@covalent.net.
> > find_start_sequence and find_end_sequence are basically the same
> > functions. The biggest differences are the values that are set in the ctx
> > structure, this could and should be handled with flags. This code is too
> > complex to duplicate.
>
> I disagree. Yes the code is similar, but there are enough differences that the
> code becomes ugly to the point of being unreadable if it is merged using
> flags etc. I know because I started with a single merged function.
>
> >
> > > if ((ctx->combined_tag = malloc (ctx->tag_length + 1)) == NULL) {
> > > return (APR_ENOMEM);
> >
> > Why are we malloc'ing here? This data (if I understand the comments) is
> > never sent to the client. This should be a palloc, or even better a
> > pcalloc.
>
> In one of my design notes I commented that I wasn't sure of the best place to
> (m|c|p)alloc from for this. My thinking was that I didn't want to grow the
> pool size by pallocing from there for something that was strictly internal
> to mod_include. My thinking was that by mallocing here I could free it when
> I am done. I also don't think this is going to happen very frequently.
You should never be malloc'ing data that isn't sent to the client. It is
asking for a memory leak.
> > get_combined_directive deals with two distinct brigades. Why aren't they
> > concatenated? This would simplify the code, and it isn't an expensive
> > operation.
>
> I thought it was expensive to setaside the rest of the brigade just so I
> could throw it away. All I am doing is copying the bytes from the buckets
> of the directive. As soon as I am done processing I throw all of those
> buckets away. Why pay the price to set aside the buckets just to throw them
> away. I don't think it adds that much complexity to handle two brigades.
It is a no-op to set-aside 99% of the buckets. The only buckets that are
expensive to set-aside are transient buckets, and the chances that you
will find a partial SSI tag in a transient bucket is slim to none. IF you
ever find an SSI tag in a transient bucket, it will be the full tag, or at
the very least, the full tag will be in the same brigade.
> > get_directive should use a hash table to store the tags. The key is the
> > directive, the value is the function to call when that directive is
> > encountered. (This also allows modules to easily extend mod_include's
> > capabilities to handle more directives, allowing us to remove
> > USE_PERL_SSI, because that becomes part of mod_perl.)
>
> I can see the added flexiblity visa-vie removing the USE_PERL_SSI stuff, but
> the problem with this is that the main send_parsed_content function does
> different wrapper code for each directive. For some it needs to check
> "printing" for others it needs to deal with "if_nesting_level", and for
> others I need to send the preceding buckets before processing the subrequest.
> Getting back a generic function pointer doesn't help this. It also doesn't
> improve performance for checking such a small number of directives.
That's where the biggest problem is. The main send_parsed_content
shouldn't be doing any of that work. All of that stuff should be done by
the handle_function. This goes back to making this a very simple module
that relies on other modules to do all the work.
> > Think of it this way. Right now, this code is not easily extensible, to
> > make it extensible is easy however. Instead of hard-coding the directives
> > that are understood, take a modular approach. Use a hash table to store
> > the directives that are understood, and just pass the processing off to
> > those functions. This allows another module to register a new SSI
> > directive easily. In order for this to work, the handle_* function needs
> > to have complete discretion over how the brigade is handled. Again, this
> > would simplify code, making this module easier to debug.
>
> Ok. I see your hash table argument now. I was leaving the wrapper code outside
> of the handle_* functions to avoid unnecessary function calls. You are
> advocating moving all of the code into the handle_* functions so that they
> are self contained for future flexibility. I can see and agree to this point.
> This is also an easy thing to fix. By the way, how do you see other modules
> to register handle_* functions with mod_include?
Mod_include should expose a simple hook and function pair. The hook
should be:
ap_hook_mod_include_cmd or something like that. When mod_include's
post_config function is called, it will call
ap_call_mod_include_cmd. This gives other modules a chance to extend
mod_include.
The function would be extend_mod_include, and it would basically just take
a key and a function pointer as arguments. It would add those to the
hash. Done.
> > In general, instead of the re-write making mod_include easier to deal
> > with, it has become a bigger monster, doubling in size and adding another
> > 600 lines of code (not counting the include file). I don't mind more
> > lines of code, I do mind when those lines of code are almost impossible to
> > wade through.
>
> A fair number of the added lines of code are comments added to try to make the
> change easier to understand. The code has grown by about 35% including the
> comments. What do you feel makes them impossible to wade through. This strikes
> me as sour grapes simply because you didn't write the code and are therefore
> unfamiliar with it.
If this was sour grapes, I wouldn't have spent an hour reviewing the code
yesterday. Nor would I have spent the two hours before the alpha trying
to debug and fix it. The problem is that when I first attacked
mod_include to get it using buckets, it took me under an hour to make the
basic changes (they weren't perfect, but they were close). This was the
first time I had ever touched mod_include. Yesterday, it took me over an
hour just to begin to figure out what the new mod_include is doing. To
me, if it takes longer to figure out this module now, then it is more
complex. This is not sour grapes, this is a fact of life.
> > I have started to try to debug this module, but I am giving up now. I am
> > unlikely to touch this version of the module again, because it is just too
> > much. I'm going back to the original version. If somebody else wants to
> > debug the new one, feel free.
>
> I didn't ask you to debug the module, I am not asking you to fix the module, and
> I sure as hell am not expecting you to rewrite the module. The comments that you
> made are all easily fixed in this code. I don't see a single one of them that
> warrants this sort of inane response. I will have a fixed patch soon addressing
> all of your comments.
But none of the comments that I made will fix the underlying problems
either. I can't find the problems with this code, and I'm sorry if you
don't like my comments. I gave my honest opinion of this module as it
stands now. My opinion may change as the code is debuged more, I don't
know if it will or not. In the meantime, I am likely to go back to
hacking the old mod_include in private, because I want a working
mod_include ASAP. I don't care if that is yours or the original, I just
want a working mod_include. If/When I get the original working, I will
post the patches to the list.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [Patch]: mod_include.c port to 2.0 buckets (includes
newmod_include.h)
Posted by "Paul J. Reder" <re...@raleigh.ibm.com>.
Ryan,
Thank you for your comments. I am sorry for the long point by point response.
I am updating my patch to address all of your comments. Please read my
response for explainations.
Thank you.
rbb@covalent.net wrote:
> ...USE_SFIO...VOIDUSED...USE_STDIO?
These were historical. I didn't notice them there.
>
> find_start_sequence and find_end_sequence are basically the same
> functions. The biggest differences are the values that are set in the ctx
> structure, this could and should be handled with flags. This code is too
> complex to duplicate.
I disagree. Yes the code is similar, but there are enough differences that the
code becomes ugly to the point of being unreadable if it is merged using
flags etc. I know because I started with a single merged function.
>
> > if ((ctx->combined_tag = malloc (ctx->tag_length + 1)) == NULL) {
> > return (APR_ENOMEM);
>
> Why are we malloc'ing here? This data (if I understand the comments) is
> never sent to the client. This should be a palloc, or even better a
> pcalloc.
In one of my design notes I commented that I wasn't sure of the best place to
(m|c|p)alloc from for this. My thinking was that I didn't want to grow the
pool size by pallocing from there for something that was strictly internal
to mod_include. My thinking was that by mallocing here I could free it when
I am done. I also don't think this is going to happen very frequently.
>
> get_combined_directive deals with two distinct brigades. Why aren't they
> concatenated? This would simplify the code, and it isn't an expensive
> operation.
I thought it was expensive to setaside the rest of the brigade just so I
could throw it away. All I am doing is copying the bytes from the buckets
of the directive. As soon as I am done processing I throw all of those
buckets away. Why pay the price to set aside the buckets just to throw them
away. I don't think it adds that much complexity to handle two brigades.
>
> get_combined_directive also uses AP_BUCKET_PREV(dptr) != bb_end. From
> reading the code, this could be simplified to
> !AP_BRIGADE_SENTINEL(dptr). This would again simplify the code and make
> it easier to understand and debug.
You are quite correct. I learned about AP_BRIGADE_SENTINEL after I wrote
this code.
>
> get_tag_and_value returns the value using return, and returns the tag in
> an argument. For consistancy it should return both as arguments.
Ok. Easily fixed.
>
> get_directive should use a hash table to store the tags. The key is the
> directive, the value is the function to call when that directive is
> encountered. (This also allows modules to easily extend mod_include's
> capabilities to handle more directives, allowing us to remove
> USE_PERL_SSI, because that becomes part of mod_perl.)
I can see the added flexiblity visa-vie removing the USE_PERL_SSI stuff, but
the problem with this is that the main send_parsed_content function does
different wrapper code for each directive. For some it needs to check
"printing" for others it needs to deal with "if_nesting_level", and for
others I need to send the preceding buckets before processing the subrequest.
Getting back a generic function pointer doesn't help this. It also doesn't
improve performance for checking such a small number of directives.
> You can't do this with any transient buckets. The point behind transient
> buckets is that they are stack data, but in order for them to work, you
> have to call the set-aside function on those buckets. If you don't, the
> data just disappears out from under the bucket. Since your data is
> disappearing immediately, this should just be put in a heep or pool
> bucket.
>
> Same problem here.
Ok. Chalk this up to ignorance on my part.
>
> You have ap_r* functions in the main-line code. This is not allowed. #1,
> a handler that uses buckets cannot also use ap_r*. #2 and more important,
> NO filter can use ap_r* functions, ever.
I assume you are referreing to ap_r(v)puts and not ap_run_sub_req or
ap_regex. I actually knew that those were problems and had it on my list
to clean up. I believe that all of those are in the parse_expr function
which I left to last to work on and then didn't get to. Those are just
leftover from before and I had already figured out how to get rid of them,
I just didn't actually do it.
> What exactly is parse_expr doing? It is a huge function, and I don't even
> want to try to figure out what it is up to.
It is doing what it did before ;) (I didn't touch it yet). It is the
expression parser for the "if" and "else" directives. I should be
able to clean it up and simplify it, but it was not broken at the moment
and so didn't need fixing (other than the ap_r* calls).
>
> LOG_COND_STATUS is broken. You can't use a transient bucket in this
> way. Same as above.
Ok. Same as above.
>
> The handle_* function abstraction is still wrong. It shouldn't be up to
> send_parsed_content to split the brigade and pass it if
> necessary. Sometimes, the include directive requires a sub_request,
> sometimes it doesn't (error condition). Why do we always split and send
> the brigade? The abstraction is simple, the handle_* function gets the
> whole brigade, and a pointer to the start of the tag. The handle_*
> function can then decide if it wants to split and pass the brigade or not.
Easy change to move this from send_parsed_content in to the handle functions.
>
> Think of it this way. Right now, this code is not easily extensible, to
> make it extensible is easy however. Instead of hard-coding the directives
> that are understood, take a modular approach. Use a hash table to store
> the directives that are understood, and just pass the processing off to
> those functions. This allows another module to register a new SSI
> directive easily. In order for this to work, the handle_* function needs
> to have complete discretion over how the brigade is handled. Again, this
> would simplify code, making this module easier to debug.
Ok. I see your hash table argument now. I was leaving the wrapper code outside
of the handle_* functions to avoid unnecessary function calls. You are
advocating moving all of the code into the handle_* functions so that they
are self contained for future flexibility. I can see and agree to this point.
This is also an easy thing to fix. By the way, how do you see other modules
to register handle_* functions with mod_include?
>
> What is FREDDY_KRUGER_VEGAMATIC_MODE?
This was testing code to pathologically slice the brigade into a bunch of
brigades with a single bucket with a single byte in it. Hence the reference
to Freddy Kruger (sp) of Nightmare on elm street fame.
>
> CREATE_ERROR_BUCKET is brokecn because of the transient problem.
Ok. Same as above.
> In general, instead of the re-write making mod_include easier to deal
> with, it has become a bigger monster, doubling in size and adding another
> 600 lines of code (not counting the include file). I don't mind more
> lines of code, I do mind when those lines of code are almost impossible to
> wade through.
A fair number of the added lines of code are comments added to try to make the
change easier to understand. The code has grown by about 35% including the
comments. What do you feel makes them impossible to wade through. This strikes
me as sour grapes simply because you didn't write the code and are therefore
unfamiliar with it.
> I have started to try to debug this module, but I am giving up now. I am
> unlikely to touch this version of the module again, because it is just too
> much. I'm going back to the original version. If somebody else wants to
> debug the new one, feel free.
I didn't ask you to debug the module, I am not asking you to fix the module, and
I sure as hell am not expecting you to rewrite the module. The comments that you
made are all easily fixed in this code. I don't see a single one of them that
warrants this sort of inane response. I will have a fixed patch soon addressing
all of your comments.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [Patch]: mod_include.c port to 2.0 buckets (includes new
mod_include.h)
Posted by rb...@covalent.net.
I'm not copying the whole file, because it is just to big. Here
are my current comments. I am sure there will be more as I review
the code further.
Why do we still have USE_SFIO in the code? There is no way to use SFIO in
Apache 2.0. What is VOIDUSED? Why do we define USE_STDIO?
find_start_sequence and find_end_sequence are basically the same
functions. The biggest differences are the values that are set in the ctx
structure, this could and should be handled with flags. This code is too
complex to duplicate.
> if ((ctx->combined_tag = malloc (ctx->tag_length + 1)) == NULL) {
> return (APR_ENOMEM);
Why are we malloc'ing here? This data (if I understand the comments) is
never sent to the client. This should be a palloc, or even better a
pcalloc.
get_combined_directive deals with two distinct brigades. Why aren't they
concatenated? This would simplify the code, and it isn't an expensive
operation.
get_combined_directive also uses AP_BUCKET_PREV(dptr) != bb_end. From
reading the code, this could be simplified to
!AP_BRIGADE_SENTINEL(dptr). This would again simplify the code and make
it easier to understand and debug.
get_tag_and_value returns the value using return, and returns the tag in
an argument. For consistancy it should return both as arguments.
get_directive should use a hash table to store the tags. The key is the
directive, the value is the function to call when that directive is
encountered. (This also allows modules to easily extend mod_include's
capabilities to handle more directives, allowing us to remove
USE_PERL_SSI, because that becomes part of mod_perl.)
> if (ap_is_HTTP_REDIRECT(rr_status)) {
> apr_ssize_t len_loc;
> const char *location = apr_table_get(rr->headers_out, "Location");
>
> location = ap_escape_html(rr->pool, location);
> len_loc = strlen(location);
>
> tmp_buck = ap_bucket_create_immortal("<A HREF=\"", sizeof("<A HREF=\""));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> tmp2_buck = ap_bucket_create_transient(location, len_loc);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_immortal("\">", sizeof("\">"));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_transient(location, len_loc);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
> tmp2_buck = ap_bucket_create_immortal("</A>", sizeof("</A>"));
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp2_buck);
>
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
> }
You can't do this with any transient buckets. The point behind transient
buckets is that they are stack data, but in order for them to work, you
have to call the set-aside function on those buckets. If you don't, the
data just disappears out from under the bucket. Since your data is
disappearing immediately, this should just be put in a heep or pool
bucket.
> parse_string(r, tag_val, parsed_string, sizeof(parsed_string), 0);
> if (!find_file(r, "flastmod", tag, parsed_string, &finfo)) {
> char *t_val;
>
> t_val = ap_ht_time(r->pool, finfo.mtime, ctx->time_str, 0);
> t_len = strlen(t_val);
>
> tmp_buck = ap_bucket_create_transient(t_val, t_len);
> AP_BUCKET_INSERT_BEFORE(head_ptr, tmp_buck);
> if (*inserted_head == NULL) {
> *inserted_head = tmp_buck;
> }
> }
Same problem here.
You have ap_r* functions in the main-line code. This is not allowed. #1,
a handler that uses buckets cannot also use ap_r*. #2 and more important,
NO filter can use ap_r* functions, ever.
What exactly is parse_expr doing? It is a huge function, and I don't even
want to try to figure out what it is up to.
LOG_COND_STATUS is broken. You can't use a transient bucket in this
way. Same as above.
The handle_* function abstraction is still wrong. It shouldn't be up to
send_parsed_content to split the brigade and pass it if
necessary. Sometimes, the include directive requires a sub_request,
sometimes it doesn't (error condition). Why do we always split and send
the brigade? The abstraction is simple, the handle_* function gets the
whole brigade, and a pointer to the start of the tag. The handle_*
function can then decide if it wants to split and pass the brigade or not.
Think of it this way. Right now, this code is not easily extensible, to
make it extensible is easy however. Instead of hard-coding the directives
that are understood, take a modular approach. Use a hash table to store
the directives that are understood, and just pass the processing off to
those functions. This allows another module to register a new SSI
directive easily. In order for this to work, the handle_* function needs
to have complete discretion over how the brigade is handled. Again, this
would simplify code, making this module easier to debug.
What is FREDDY_KRUGER_VEGAMATIC_MODE?
CREATE_ERROR_BUCKET is brokecn because of the transient problem.
Other issues that are not related to mod_include:
While testing this, I found a seg-fault when requesting pages with
IE. This seg-fault was cured when I removed the coalesce filter.
In general, instead of the re-write making mod_include easier to deal
with, it has become a bigger monster, doubling in size and adding another
600 lines of code (not counting the include file). I don't mind more
lines of code, I do mind when those lines of code are almost impossible to
wade through.
I have started to try to debug this module, but I am giving up now. I am
unlikely to touch this version of the module again, because it is just too
much. I'm going back to the original version. If somebody else wants to
debug the new one, feel free.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------