[PATCH 3 of 3] Headers filter: added "add_trailer" directive

Maxim Dounin mdounin at mdounin.ru
Mon Jun 5 17:52:45 UTC 2017


Hello!

On Fri, Jun 02, 2017 at 08:33:47PM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora at google.com>
> # Date 1490351854 25200
> #      Fri Mar 24 03:37:34 2017 -0700
> # Node ID acdc80c0d4ef8aa2519e2882ff1a3bd4a316ad81
> # Parent  8d74ff6c2015180f5c1f399f492214d7d0a52b3f
> Headers filter: added "add_trailer" directive.
> 
> Trailers added using this directive are evaluated after response body
> is processed by output filters (but before it's written to the wire),
> so it's possible to use variables calculated from the response body
> as the trailer value.
> 
> Signed-off-by: Piotr Sikora <piotrsikora at google.com>

Overall looks good, see some comments below.

[...]

> @@ -206,11 +223,103 @@ ngx_http_headers_filter(ngx_http_request
>          }
>      }
>  
> +    if (conf->trailers) {
> +        h = conf->trailers->elts;
> +        for (i = 0; i < conf->trailers->nelts; i++) {
> +
> +            if (!safe_status && !h[i].always) {
> +                continue;
> +            }
> +
> +            if (h[i].value.value.len) {
> +                r->expect_trailers = 1;
> +                break;
> +            }

It doesn't look like "if (h[i].value.value.len)" is needed here.  
It is either true, or the "add_trailer" directive is nop and we 
already know this while parsing the configuration.

-            if (h[i].value.value.len) {
-                r->expect_trailers = 1;
-                break;
-            }
+            r->expect_trailers = 1;
+            break;
 
[...]

> @@ -741,3 +858,63 @@ ngx_http_headers_add(ngx_conf_t *cf, ngx
>  
>      return NGX_CONF_OK;
>  }
> +
> +
> +static char *
> +ngx_http_headers_add_trailer(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> +{
> +    ngx_http_headers_conf_t *hcf = conf;
> +
> +    ngx_str_t                         *value;
> +    ngx_http_header_val_t             *hv;
> +    ngx_http_compile_complex_value_t   ccv;
> +
> +    value = cf->args->elts;
> +
> +    if (hcf->trailers == NULL) {
> +        hcf->trailers = ngx_array_create(cf->pool, 1,
> +                                         sizeof(ngx_http_header_val_t));
> +        if (hcf->trailers == NULL) {
> +            return NGX_CONF_ERROR;
> +        }
> +    }
> +
> +    hv = ngx_array_push(hcf->trailers);
> +    if (hv == NULL) {
> +        return NGX_CONF_ERROR;
> +    }

[...]

It looks like the ngx_http_headers_add_trailer() function is 
almost exact copy of the ngx_http_headers_add().  It might worth 
to use single function to handle both directives.

diff --git a/src/http/modules/ngx_http_headers_filter_module.c b/src/http/modules/ngx_http_headers_filter_module.c
--- a/src/http/modules/ngx_http_headers_filter_module.c
+++ b/src/http/modules/ngx_http_headers_filter_module.c
@@ -73,8 +73,6 @@ static char *ngx_http_headers_expires(ng
     void *conf);
 static char *ngx_http_headers_add(ngx_conf_t *cf, ngx_command_t *cmd,
     void *conf);
-static char *ngx_http_headers_add_trailer(ngx_conf_t *cf, ngx_command_t *cmd,
-    void *conf);
 
 
 static ngx_http_set_header_t  ngx_http_set_headers[] = {
@@ -108,15 +106,15 @@ static ngx_command_t  ngx_http_headers_f
                         |NGX_CONF_TAKE23,
       ngx_http_headers_add,
       NGX_HTTP_LOC_CONF_OFFSET,
-      0,
+      offsetof(ngx_http_headers_conf_t, headers),
       NULL },
 
     { ngx_string("add_trailer"),
       NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_HTTP_LIF_CONF
                         |NGX_CONF_TAKE23,
-      ngx_http_headers_add_trailer,
+      ngx_http_headers_add,
       NGX_HTTP_LOC_CONF_OFFSET,
-      0,
+      offsetof(ngx_http_headers_conf_t, trailers),
       NULL },
 
       ngx_null_command
@@ -231,10 +229,8 @@ ngx_http_headers_filter(ngx_http_request
                 continue;
             }
 
-            if (h[i].value.value.len) {
-                r->expect_trailers = 1;
-                break;
-            }
+            r->expect_trailers = 1;
+            break;
         }
     }
 
@@ -791,42 +787,49 @@ ngx_http_headers_add(ngx_conf_t *cf, ngx
 {
     ngx_http_headers_conf_t *hcf = conf;
 
-    ngx_str_t                         *value;
-    ngx_uint_t                         i;
-    ngx_http_header_val_t             *hv;
-    ngx_http_set_header_t             *set;
-    ngx_http_compile_complex_value_t   ccv;
+    ngx_str_t                          *value;
+    ngx_uint_t                          i;
+    ngx_array_t                       **headers;
+    ngx_http_header_val_t              *hv;
+    ngx_http_set_header_t              *set;
+    ngx_http_compile_complex_value_t    ccv;
 
     value = cf->args->elts;
 
-    if (hcf->headers == NULL) {
-        hcf->headers = ngx_array_create(cf->pool, 1,
-                                        sizeof(ngx_http_header_val_t));
-        if (hcf->headers == NULL) {
+    headers = (ngx_array_t **) ((char *) hcf + cmd->offset);
+
+    if (*headers == NULL) {
+        *headers = ngx_array_create(cf->pool, 1,
+                                    sizeof(ngx_http_header_val_t));
+        if (*headers == NULL) {
             return NGX_CONF_ERROR;
         }
     }
 
-    hv = ngx_array_push(hcf->headers);
+    hv = ngx_array_push(*headers);
     if (hv == NULL) {
         return NGX_CONF_ERROR;
     }
 
     hv->key = value[1];
-    hv->handler = ngx_http_add_header;
+    hv->handler = NULL;
     hv->offset = 0;
     hv->always = 0;
 
-    set = ngx_http_set_headers;
-    for (i = 0; set[i].name.len; i++) {
-        if (ngx_strcasecmp(value[1].data, set[i].name.data) != 0) {
-            continue;
+    if (headers == &hcf->headers) {
+        hv->handler = ngx_http_add_header;
+
+        set = ngx_http_set_headers;
+        for (i = 0; set[i].name.len; i++) {
+            if (ngx_strcasecmp(value[1].data, set[i].name.data) != 0) {
+                continue;
+            }
+
+            hv->offset = set[i].offset;
+            hv->handler = set[i].handler;
+
+            break;
         }
-
-        hv->offset = set[i].offset;
-        hv->handler = set[i].handler;
-
-        break;
     }
 
     if (value[2].len == 0) {
@@ -858,63 +861,3 @@ ngx_http_headers_add(ngx_conf_t *cf, ngx
 
     return NGX_CONF_OK;
 }
-
-
-static char *
-ngx_http_headers_add_trailer(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
-{
-    ngx_http_headers_conf_t *hcf = conf;
-
-    ngx_str_t                         *value;
-    ngx_http_header_val_t             *hv;
-    ngx_http_compile_complex_value_t   ccv;
-
-    value = cf->args->elts;
-
-    if (hcf->trailers == NULL) {
-        hcf->trailers = ngx_array_create(cf->pool, 1,
-                                         sizeof(ngx_http_header_val_t));
-        if (hcf->trailers == NULL) {
-            return NGX_CONF_ERROR;
-        }
-    }
-
-    hv = ngx_array_push(hcf->trailers);
-    if (hv == NULL) {
-        return NGX_CONF_ERROR;
-    }
-
-    hv->key = value[1];
-    hv->handler = NULL;
-    hv->offset = 0;
-    hv->always = 0;
-
-    if (value[2].len == 0) {
-        ngx_memzero(&hv->value, sizeof(ngx_http_complex_value_t));
-
-    } else {
-        ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
-
-        ccv.cf = cf;
-        ccv.value = &value[2];
-        ccv.complex_value = &hv->value;
-
-        if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
-            return NGX_CONF_ERROR;
-        }
-    }
-
-    if (cf->args->nelts == 3) {
-        return NGX_CONF_OK;
-    }
-
-    if (ngx_strcmp(value[3].data, "always") != 0) {
-        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
-                           "invalid parameter \"%V\"", &value[3]);
-        return NGX_CONF_ERROR;
-    }
-
-    hv->always = 1;
-
-    return NGX_CONF_OK;
-}


-- 
Maxim Dounin
http://nginx.org/


More information about the nginx-devel mailing list