Hello!

On Wed, May 11, 2022 at 11:21:15PM +0400, Sergey Kandaurov wrote:

> On Thu, Apr 21, 2022 at 01:18:46AM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin <mdounin@mdounin.ru>
> > # Date 1650492323 -10800
> > # Thu Apr 21 01:05:23 2022 +0300
> > # Node ID 50fe52f516ff9c148aa9e7dfcc1c31cc6a4929ae
> > # Parent 2ea48b5e4643a818cd81179f040f0b36be9050d6
> > Reworked multi headers to use linked lists.
> >
> > Multi headers are now using linked lists instead of arrays. Notably,
> > the following fields were changed: r->headers_in.cookies (renamed
> > to r->headers_in.cookie), r->headers_in.x_forwarded_for,
> > r->headers_out.cache_control, r->headers_out.link, u->headers_in.cache_control
> > u->headers_in.cookies (renamed to u->headers_in.set_cookie).
>
> jftr, this implies updating a bunch of 3rd party modules, including njs, lua,
> naxsi, nginx-clojure, headers-more, geoip2_module, ngx_http_sticky_module

Sure.

> >
> > The r->headers_in.cookies and u->headers_in.cookies fields were renamed
> > to r->headers_in.cookie and u->headers_in.set_cookie to match header names.
> >
> > The ngx_http_parse_multi_header_lines() and ngx_http_parse_set_cookie_lines()
> > functions were changed accordingly.
> >
> > With this change, multi headers are now essentially equivalent to normal
> > headers, and following changes will further make them equivalent.
> >
> > diff --git a/src/http/modules/ngx_http_geo_module.c b/src/http/modules/ngx_http_geo_module.c
> > --- a/src/http/modules/ngx_http_geo_module.c
> > +++ b/src/http/modules/ngx_http_geo_module.c
> > @@ -327,15 +327,15 @@ static ngx_int_t
> > ngx_http_geo_addr(ngx_http_request_t *r, ngx_http_geo_ctx_t *ctx,
> > ngx_addr_t *addr)
> > {
> > - ngx_array_t *xfwd;
> > + ngx_table_elt_t *xfwd;
> >
> > if (ngx_http_geo_real_addr(r, ctx, addr) != NGX_OK) {
> > return NGX_ERROR;
> > }
> >
> > - xfwd = &r->headers_in.x_forwarded_for;
> > + xfwd = r->headers_in.x_forwarded_for;
> >
> > - if (xfwd->nelts > 0 && ctx->proxies != NULL) {
> > + if (xfwd != NULL && ctx->proxies != NULL) {
> > (void) ngx_http_get_forwarded_addr(r, addr, xfwd, NULL,
> > ctx->proxies, ctx->proxy_recursive);
> > }
> > diff --git a/src/http/modules/ngx_http_geoip_module.c b/src/http/modules/ngx_http_geoip_module.c
> > --- a/src/http/modules/ngx_http_geoip_module.c
> > +++ b/src/http/modules/ngx_http_geoip_module.c
> > @@ -240,16 +240,16 @@ static u_long
> > ngx_http_geoip_addr(ngx_http_request_t *r, ngx_http_geoip_conf_t *gcf)
> > {
> > ngx_addr_t addr;
> > - ngx_array_t *xfwd;
> > + ngx_table_elt_t *xfwd;
> > struct sockaddr_in *sin;
> >
> > addr.sockaddr = r->connection->sockaddr;
> > addr.socklen = r->connection->socklen;
> > /* addr.name = r->connection->addr_text; */
> >
> > - xfwd = &r->headers_in.x_forwarded_for;
> > + xfwd = r->headers_in.x_forwarded_for;
> >
> > - if (xfwd->nelts > 0 && gcf->proxies != NULL) {
> > + if (xfwd != NULL && gcf->proxies != NULL) {
> > (void) ngx_http_get_forwarded_addr(r, &addr, xfwd, NULL,
> > gcf->proxies, gcf->proxy_recursive);
> > }
> > @@ -292,7 +292,7 @@ static geoipv6_t
> > ngx_http_geoip_addr_v6(ngx_http_request_t *r, ngx_http_geoip_conf_t *gcf)
> > {
> > ngx_addr_t addr;
> > - ngx_array_t *xfwd;
> > + ngx_table_elt_t *xfwd;
> > in_addr_t addr4;
> > struct in6_addr addr6;
> > struct sockaddr_in *sin;
> > @@ -302,9 +302,9 @@ ngx_http_geoip_addr_v6(ngx_http_request_
> > addr.socklen = r->connection->socklen;
> > /* addr.name = r->connection->addr_text; */
> >
> > - xfwd = &r->headers_in.x_forwarded_for;
> > + xfwd = r->headers_in.x_forwarded_for;
> >
> > - if (xfwd->nelts > 0 && gcf->proxies != NULL) {
> > + if (xfwd != NULL && gcf->proxies != NULL) {
> > (void) ngx_http_get_forwarded_addr(r, &addr, xfwd, NULL,
> > gcf->proxies, gcf->proxy_recursive);
> > }
> > 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
> > @@ -329,8 +329,7 @@ ngx_http_set_expires(ngx_http_request_t
> > time_t now, expires_time, max_age;
> > ngx_str_t value;
> > ngx_int_t rc;
> > - ngx_uint_t i;
> > - ngx_table_elt_t *e, *cc, **ccp;
> > + ngx_table_elt_t *e, *cc;
> > ngx_http_expires_t expires;
> >
> > expires = conf->expires;
> > @@ -371,38 +370,28 @@ ngx_http_set_expires(ngx_http_request_t
> > len = sizeof("Mon, 28 Sep 1970 06:00:00 GMT");
> > e->value.len = len - 1;
> >
> > - ccp = r->headers_out.cache_control.elts;
> > -
> > - if (ccp == NULL) {
> > + cc = r->headers_out.cache_control;
> >
> > - if (ngx_array_init(&r->headers_out.cache_control, r->pool,
> > - 1, sizeof(ngx_table_elt_t *))
> > - != NGX_OK)
> > - {
> > - return NGX_ERROR;
> > - }
> > + if (cc == NULL) {
> >
> > cc = ngx_list_push(&r->headers_out.headers);
> > if (cc == NULL) {
> > return NGX_ERROR;
> > }
> >
> > + r->headers_out.cache_control = cc;
> > + cc->next = NULL;
> > +
> > cc->hash = 1;
> > ngx_str_set(&cc->key, "Cache-Control");
> >
> > - ccp = ngx_array_push(&r->headers_out.cache_control);
> > - if (ccp == NULL) {
> > - return NGX_ERROR;
> > + } else {
> > + for (cc = cc->next; cc; cc = cc->next) {
> > + cc->hash = 0;
> > }
> >
> > - *ccp = cc;
> > -
> > - } else {
> > - for (i = 1; i < r->headers_out.cache_control.nelts; i++) {
> > - ccp[i]->hash = 0;
> > - }
> > -
> > - cc = ccp[0];
> > + cc = r->headers_out.cache_control;
> > + cc->next = NULL;
> > }
> >
> > if (expires == NGX_HTTP_EXPIRES_EPOCH) {
> > @@ -564,22 +553,12 @@ static ngx_int_t
> > ngx_http_add_multi_header_lines(ngx_http_request_t *r,
> > ngx_http_header_val_t *hv, ngx_str_t *value)
> > {
> > - ngx_array_t *pa;
> > ngx_table_elt_t *h, **ph;
> >
> > if (value->len == 0) {
> > return NGX_OK;
> > }
> >
> > - pa = (ngx_array_t *) ((char *) &r->headers_out + hv->offset);
> > -
> > - if (pa->elts == NULL) {
> > - if (ngx_array_init(pa, r->pool, 1, sizeof(ngx_table_elt_t *)) != NGX_OK)
> > - {
> > - return NGX_ERROR;
> > - }
> > - }
> > -
> > h = ngx_list_push(&r->headers_out.headers);
> > if (h == NULL) {
> > return NGX_ERROR;
> > @@ -589,12 +568,12 @@ ngx_http_add_multi_header_lines(ngx_http
> > h->key = hv->key;
> > h->value = *value;
> >
> > - ph = ngx_array_push(pa);
> > - if (ph == NULL) {
> > - return NGX_ERROR;
> > - }
> > + ph = (ngx_table_elt_t **) ((char *) &r->headers_out + hv->offset);
> > +
> > + while (*ph) { ph = &(*ph)->next; }
> >
> > *ph = h;
> > + h->next = NULL;
> >
> > return NGX_OK;
> > }
> > diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
> > --- a/src/http/modules/ngx_http_proxy_module.c
> > +++ b/src/http/modules/ngx_http_proxy_module.c
> > @@ -2559,22 +2559,20 @@ static ngx_int_t
> > ngx_http_proxy_add_x_forwarded_for_variable(ngx_http_request_t *r,
> > ngx_http_variable_value_t *v, uintptr_t data)
> > {
> > - size_t len;
> > - u_char *p;
> > - ngx_uint_t i, n;
> > - ngx_table_elt_t **h;
> > + size_t len;
> > + u_char *p;
> > + ngx_table_elt_t *h, *xfwd;
> >
> > v->valid = 1;
> > v->no_cacheable = 0;
> > v->not_found = 0;
> >
> > - n = r->headers_in.x_forwarded_for.nelts;
> > - h = r->headers_in.x_forwarded_for.elts;
> > + xfwd = r->headers_in.x_forwarded_for;
> >
> > len = 0;
> >
> > - for (i = 0; i < n; i++) {
> > - len += h[i]->value.len + sizeof(", ") - 1;
> > + for (h = xfwd; h; h = h->next) {
> > + len += h->value.len + sizeof(", ") - 1;
> > }
> >
> > if (len == 0) {
>
> The only minor point in a whole patch (which I won't insist):
> a special case for xfwd absense can be slightly reshuffled now
> to avoid a do-nothing loop.
>
> diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -2569,18 +2569,18 @@ ngx_http_proxy_add_x_forwarded_for_varia
>
> xfwd = r->headers_in.x_forwarded_for;
>
> + if (xfwd == NULL) {
> + v->len = r->connection->addr_text.len;
> + v->data = r->connection->addr_text.data;
> + return NGX_OK;
> + }
> +
> len = 0;
>
> for (h = xfwd; h; h = h->next) {
> len += h->value.len + sizeof(", ") - 1;
> }
>
> - if (len == 0) {
> - v->len = r->connection->addr_text.len;
> - v->data = r->connection->addr_text.data;
> - return NGX_OK;
> - }
> -
> len += r->connection->addr_text.len;
>
> p = ngx_pnalloc(r->pool, len);
>
> Otherwise, looks good.

Such an optimization (if at all) is completely orthogonal and
available regardless of the r->headers_in.x_forwarded_for format
(that is, nothing stops you from testing "n == 0" in the original
code).

Either way, I don't think such a change belongs to this patch
and/or this patch series.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org