[PATCH] httpd - User defined error pages

[Pierre Métras]

Hi Denis,

> > This patch includes:
> > - Support for definition of error pages in httpd.conf (it was the main
> > goal!). This is enabled by a configuration option, default no.
> > - Reports of simple errors in httpd.conf when they are detected. It does
> > not check that the error page exists. Reports of memory exhausted.
>
> Please rediff against current svn and resend. Unfortunately it collides
> with your other patch (sendfile support) which I just applied with changes.


Before I reapply the patch to the current SVN sources and do some changes, I 
want to answer your concerns about the changes:

> > - Support for comments and empty lines in httpd.conf. End of line
> > comments where already supported and not documented.
>
> Can you submit it as separate patch? (Two patches in one mail is ok with
> me).

20-30% of the changes in the patch file are about reformating of the code 
(missing tab after a {-less if) or adding comments to document the code. Full 
support for comments in httpd.conf is only 4 extra lines, after I discovered 
that this feature was already implemented but not documented.
Syntax error messages are also 2 added lines...

>
> Review:
>
> -static const HttpEnumString httpResponseNames[] = {
> -       { HTTP_OK, "OK", NULL },
> -       { HTTP_MOVED_TEMPORARILY, "Found", "Directories must end with a
> slash." }, +static SKIP_FEATURE_HTTPD_ERROR_PAGES(const) HttpEnumString
> httpResponseNames[] = { +       { HTTP_OK, "OK", NULL
> USE_FEATURE_HTTPD_ERROR_PAGES(, NULL) }, +       { HTTP_MOVED_TEMPORARILY,
> "Found", "Directories must end with a slash."
> USE_FEATURE_HTTPD_ERROR_PAGES(, NULL) },
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ IIRC you can simply omit NULL/0
> trailing members in struct initializer. Will look much less ugly here.

I know, but without the USE_FEATURE_HTTPD_ERROR_PAGES macro, we could have 
compilation error (best case) or bugs in the future if someone adds a new 
member to the struct. That's the reason I've used it, but I can remove it 
from the patch if you want...

>
>
> @@ -488,22 +508,30 @@
>  #endif
>         /* This could stand some work */
>         while ((p0 = fgets(buf, sizeof(buf), f)) != NULL) {
> +               /* Comments: '#' as first character of the line */
> +               if (*p0 == '#' || *p0 == '\n')
> +                       continue;
> +

The 4 lines talked about before, for comment lines.

>                 c = NULL;
>                 for (p = p0; *p0 != 0 && *p0 != '#'; p0++) {
>                         if (!isspace(*p0)) {
>                                 *p++ = *p0;
>                                 if (*p0 == ':' && c == NULL)
> -                               c = p;
> +                                       c = p;

Reformating the source...

>                         }
>                 }
>                 *p = 0;
>
> -               /* test for empty or strange line */
> -               if (c == NULL || *c == 0)
> +               /* test for empty or strange line
> +                  ':' is present on all syntaxes
> +                  and c points at the char after it */
> +               if (c == NULL || *c == 0) {
> +                       bb_error_msg("config error '%s' in '%s'", buf, cf);
>                         continue;
> +               }

A better syntax error message when an error is discovered in httpd.conf 
instead of silently ignoring it.

>                 p0 = buf;
>                 if (*p0 == 'd')
> -                               *p0 = 'D';
> +                       *p0 = 'D';
>                 if (*c == '*') {
>                         if (*p0 == 'D') {
>                                 /* memorize deny all */
> @@ -515,18 +543,6 @@
>
>                 if (*p0 == 'a')
>                         *p0 = 'A';
> -               else if (*p0 != 'D' && *p0 != 'A'
> -#if ENABLE_FEATURE_HTTPD_BASIC_AUTH
> -                        && *p0 != '/'
> -#endif
> -#if ENABLE_FEATURE_HTTPD_CONFIG_WITH_MIME_TYPES
> -                        && *p0 != '.'
> -#endif
> -#if ENABLE_FEATURE_HTTPD_CONFIG_WITH_SCRIPT_INTERPR
> -                        && *p0 != '*'
> -#endif
> -                       )
> -                        continue;

These checks are already done later in the code, so I think we can safely 
remove them. In that block, we are parsing the Allow/Deny lines, and we don't 
need to check for the other type of lines (MIME types, user authorizations, 
etc).

>                 if (*p0 == 'A' || *p0 == 'D') {
>                         /* storing current config IP line */
>                         pip = xzalloc(sizeof(Htaccess_IP));
>
> Above two hunks are not immediately obvious. What do they do?
> If it's unrelated to error pages support, submit separately.
> --
> vda

In the end, my patch was 80% about support for error pages, and 20% about 
adding comments/documentation/reformating of the code.
Do you prefer splitted patches: one for error pages strictly, one for 
comments, one for reformated source?

-- Pierre Métras