> On 4 Jun 2023, at 06:20, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Thu, Jun 01, 2023 at 05:58:38AM +0400, Sergey Kandaurov wrote:
>
>> # HG changeset patch
>> # User Sergey Kandaurov <pluknet@nginx.com>
>> # Date 1685584419 -14400
>> # Thu Jun 01 05:53:39 2023 +0400
>> # Node ID cd90a7bed6ebb098efe1e82b7ffa067e5f0e56c1
>> # Parent 79ed88b1bf961a19b1efccd058fae440c3acbbdc
>> HTTP/2: removed server push.
>>
>> Although it has better implementation status than HTTP/3 server push,
>> it remains of limited use, with adoption numbers seen as negligible.
>> Per IETF 102 materials, server push was used only in 0.04% of sessions.
>> It was considered to be "difficult to use effectively" in RFC 9113.
>> Its use is further limited by badly matching to fetch/cache/connection
>> models in browsers, see related discussions linked from [1].
>>
>> Server push was disabled in Chrome 106 [2].
>>
>> The http2_push, http2_push_preload, and http2_max_concurrent_pushes
>> directives are made obsolete.
>>
>> [1] https://jakearchibald.com/2017/h2-push-tougher-than-i-thought/
>> [2] https://chromestatus.com/feature/6302414934114304
>
> It might make sense to mention ticket #2432.
>
> It might also make sense to mention changesets essentially
> reverted, notably 7201:641306096f5b and 7207:3d2b0b02bd3d.

Applied, thanks.

>
>>
>> diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
>> --- a/src/http/v2/ngx_http_v2.c
>> +++ b/src/http/v2/ngx_http_v2.c
>> @@ -11,14 +11,6 @@
>> #include <ngx_http_v2_module.h>
>>
>>
>> -typedef struct {
>> - ngx_str_t name;
>> - ngx_uint_t offset;
>> - ngx_uint_t hash;
>> - ngx_http_header_t *hh;
>> -} ngx_http_v2_parse_header_t;
>> -
>> -
>> /* errors */
>
> Looking into 7207:3d2b0b02bd3d, where this chunk of code was
> originally introduced, suggests that NGX_HTTP_HEADERS can be
> removed from the relevant if in auto/modules.
>
> And the same applies to HTTP/3 case as well.

Removed in both places.

>
>> #define NGX_HTTP_V2_NO_ERROR 0x0
>> #define NGX_HTTP_V2_PROTOCOL_ERROR 0x1
>> @@ -126,7 +118,7 @@ static ngx_int_t ngx_http_v2_parse_int(n
>> u_char **pos, u_char *end, ngx_uint_t prefix);
>>
>> static ngx_http_v2_stream_t *ngx_http_v2_create_stream(
>> - ngx_http_v2_connection_t *h2c, ngx_uint_t push);
>> + ngx_http_v2_connection_t *h2c);
>> static ngx_http_v2_node_t *ngx_http_v2_get_node_by_id(
>> ngx_http_v2_connection_t *h2c, ngx_uint_t sid, ngx_uint_t alloc);
>> static ngx_http_v2_node_t *ngx_http_v2_get_closed_node(
>> @@ -162,14 +154,11 @@ static ngx_int_t ngx_http_v2_parse_schem
>> ngx_str_t *value);
>> static ngx_int_t ngx_http_v2_parse_authority(ngx_http_request_t *r,
>> ngx_str_t *value);
>> -static ngx_int_t ngx_http_v2_parse_header(ngx_http_request_t *r,
>> - ngx_http_v2_parse_header_t *header, ngx_str_t *value);
>> static ngx_int_t ngx_http_v2_construct_request_line(ngx_http_request_t *r);
>> static ngx_int_t ngx_http_v2_cookie(ngx_http_request_t *r,
>> ngx_http_v2_header_t *header);
>> static ngx_int_t ngx_http_v2_construct_cookie_header(ngx_http_request_t *r);
>> static void ngx_http_v2_run_request(ngx_http_request_t *r);
>> -static void ngx_http_v2_run_request_handler(ngx_event_t *ev);
>> static ngx_int_t ngx_http_v2_process_request_body(ngx_http_request_t *r,
>> u_char *pos, size_t size, ngx_uint_t last, ngx_uint_t flush);
>> static ngx_int_t ngx_http_v2_filter_request_body(ngx_http_request_t *r);
>> @@ -210,23 +199,6 @@ static ngx_http_v2_handler_pt ngx_http_v
>> (sizeof(ngx_http_v2_frame_states) / sizeof(ngx_http_v2_handler_pt))
>>
>>
>> -static ngx_http_v2_parse_header_t ngx_http_v2_parse_headers[] = {
>> - { ngx_string("host"),
>> - offsetof(ngx_http_headers_in_t, host), 0, NULL },
>> -
>> - { ngx_string("accept-encoding"),
>> - offsetof(ngx_http_headers_in_t, accept_encoding), 0, NULL },
>> -
>> - { ngx_string("accept-language"),
>> - offsetof(ngx_http_headers_in_t, accept_language), 0, NULL },
>> -
>> - { ngx_string("user-agent"),
>> - offsetof(ngx_http_headers_in_t, user_agent), 0, NULL },
>> -
>> - { ngx_null_string, 0, 0, NULL }
>> -};
>> -
>> -
>> void
>> ngx_http_v2_init(ngx_event_t *rev)
>> {
>> @@ -275,7 +247,6 @@ ngx_http_v2_init(ngx_event_t *rev)
>>
>> h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
>>
>> - h2c->concurrent_pushes = h2scf->concurrent_pushes;
>> h2c->priority_limit = ngx_max(h2scf->concurrent_streams, 100);
>>
>> h2c->pool = ngx_create_pool(h2scf->pool_size, h2c->connection->log);
>> @@ -384,7 +355,7 @@ ngx_http_v2_read_handler(ngx_event_t *re
>> return;
>> }
>>
>> - if (!h2c->processing && !h2c->pushing) {
>> + if (!h2c->processing) {
>> ngx_http_v2_finalize_connection(h2c, NGX_HTTP_V2_NO_ERROR);
>> return;
>> }
>> @@ -427,8 +398,7 @@ ngx_http_v2_read_handler(ngx_event_t *re
>> break;
>> }
>>
>> - if (n == 0
>> - && (h2c->state.incomplete || h2c->processing || h2c->pushing))
>> + if (n == 0 && (h2c->state.incomplete || h2c->processing))
>> {
>> ngx_log_error(NGX_LOG_INFO, c->log, 0,
>> "client prematurely closed connection");
>
> Nitpicking: "{" should be on the same line with "if" (and that's
> how it was before 7201:641306096f5b).

Indeed, that was an unfinished change, fixed.

>
>> @@ -652,7 +622,7 @@ ngx_http_v2_handle_connection(ngx_http_v
>> ngx_connection_t *c;
>> ngx_http_core_loc_conf_t *clcf;
>>
>> - if (h2c->last_out || h2c->processing || h2c->pushing) {
>> + if (h2c->last_out || h2c->processing) {
>> return;
>> }
>>
>> @@ -1337,7 +1307,7 @@ ngx_http_v2_state_headers(ngx_http_v2_co
>> h2c->closed_nodes--;
>> }
>>
>> - stream = ngx_http_v2_create_stream(h2c, 0);
>> + stream = ngx_http_v2_create_stream(h2c);
>> if (stream == NULL) {
>> return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_INTERNAL_ERROR);
>> }
>
> In 7201:641306096f5b, NGX_HTTP_V2_REFUSED_STREAM handling was
> added to ngx_http_v2_state_rst_stream():
>
> @@ -1909,6 +1913,11 @@ ngx_http_v2_state_rst_stream(ngx_http_v2
> "client canceled stream %ui", h2c->state.sid);
> break;
>
> + case NGX_HTTP_V2_REFUSED_STREAM:
> + ngx_log_error(NGX_LOG_INFO, fc->log, 0,
> + "client refused stream %ui", h2c->state.sid);
> + break;
> +
> case NGX_HTTP_V2_INTERNAL_ERROR:
> ngx_log_error(NGX_LOG_INFO, fc->log, 0,
> "client terminated stream %ui due to internal error",
>
> It does not seem to make any sense now, shouldn't it be removed as
> well?

Well, it served little to no sense before.
This case is used to make distinguished logging for REFUSED_STREAM,
which is not closely related to server push being removed.
RFC 9113, 8.7 gives the following explanation:

: The REFUSED_STREAM error code can be included in a RST_STREAM
: frame to indicate that the stream is being closed prior to any
: processing having occurred. Any request that was sent on the reset
: stream can be safely retried.

That's why I refrained from removing this chunk as something related.

Though, in 8.4.2, REFUSED_STREAM is specified to cancel pushed responses,
and no other use-cases applicable seems to be related to client's usage.
So indeed it looks to be fine to be removed as well, applied.

>
>> @@ -2198,7 +2168,6 @@ ngx_http_v2_state_settings_params(ngx_ht
>> {
>> ssize_t window_delta;
>> ngx_uint_t id, value;
>> - ngx_http_v2_srv_conf_t *h2scf;
>> ngx_http_v2_out_frame_t *frame;
>>
>> window_delta = 0;
>> @@ -2260,14 +2229,6 @@ ngx_http_v2_state_settings_params(ngx_ht
>> NGX_HTTP_V2_PROTOCOL_ERROR);
>> }
>>
>> - h2c->push_disabled = !value;
>> - break;
>> -
>> - case NGX_HTTP_V2_MAX_STREAMS_SETTING:
>> - h2scf = ngx_http_get_module_srv_conf(h2c->http_connection->conf_ctx,
>> - ngx_http_v2_module);
>> -
>> - h2c->concurrent_pushes = ngx_min(value, h2scf->concurrent_pushes);
>> break;
>>
>> case NGX_HTTP_V2_HEADER_TABLE_SIZE_SETTING:
>
> Any specific reasons to keep NGX_HTTP_V2_ENABLE_PUSH_SETTING
> handling?

Note that here remains a degenerate handling, used to reject invalid
values for SETTINGS_ENABLE_PUSH. IMHO, it makes sense to keep it, since
"Any value other than 0 or 1 MUST be treated as a connection error".

>
>> @@ -2722,163 +2683,6 @@ ngx_http_v2_parse_int(ngx_http_v2_connec
>> }
>>
>>
>> -ngx_http_v2_stream_t *
>> -ngx_http_v2_push_stream(ngx_http_v2_stream_t *parent, ngx_str_t *path)
>> -{
>> - ngx_int_t rc;
>> - ngx_str_t value;
>> - ngx_pool_t *pool;
>> - ngx_uint_t index;
>> - ngx_table_elt_t **h;
>> - ngx_connection_t *fc;
>> - ngx_http_request_t *r;
>> - ngx_http_v2_node_t *node;
>> - ngx_http_v2_stream_t *stream;
>> - ngx_http_v2_srv_conf_t *h2scf;
>> - ngx_http_v2_connection_t *h2c;
>> - ngx_http_v2_parse_header_t *header;
>> -
>> - h2c = parent->connection;
>> -
>> - pool = ngx_create_pool(1024, h2c->connection->log);
>> - if (pool == NULL) {
>> - goto rst_stream;
>> - }
>> -
>> - node = ngx_http_v2_get_node_by_id(h2c, h2c->last_push, 1);
>> -
>> - if (node == NULL) {
>> - ngx_destroy_pool(pool);
>> - goto rst_stream;
>> - }
>> -
>> - stream = ngx_http_v2_create_stream(h2c, 1);
>> - if (stream == NULL) {
>> -
>> - if (node->parent == NULL) {
>> - h2scf = ngx_http_get_module_srv_conf(h2c->http_connection->conf_ctx,
>> - ngx_http_v2_module);
>> -
>> - index = ngx_http_v2_index(h2scf, h2c->last_push);
>> - h2c->streams_index[index] = node->index;
>> -
>> - ngx_queue_insert_tail(&h2c->closed, &node->reuse);
>> - h2c->closed_nodes++;
>> - }
>> -
>> - ngx_destroy_pool(pool);
>> - goto rst_stream;
>> - }
>> -
>> - if (node->parent) {
>> - ngx_queue_remove(&node->reuse);
>> - h2c->closed_nodes--;
>> - }
>> -
>> - stream->pool = pool;
>> -
>> - r = stream->request;
>> - fc = r->connection;
>> -
>> - stream->in_closed = 1;
>> - stream->node = node;
>> -
>> - node->stream = stream;
>> -
>> - ngx_log_debug2(NGX_LOG_DEBUG_HTTP, h2c->connection->log, 0,
>> - "http2 push stream sid:%ui "
>> - "depends on %ui excl:0 weight:16",
>> - h2c->last_push, parent->node->id);
>> -
>> - node->weight = NGX_HTTP_V2_DEFAULT_WEIGHT;
>> - ngx_http_v2_set_dependency(h2c, node, parent->node->id, 0);
>> -
>> - r->method_name = ngx_http_core_get_method;
>> - r->method = NGX_HTTP_GET;
>> -
>> - r->schema.data = ngx_pstrdup(pool, &parent->request->schema);
>> - if (r->schema.data == NULL) {
>> - goto close;
>> - }
>> -
>> - r->schema.len = parent->request->schema.len;
>> -
>> - value.data = ngx_pstrdup(pool, path);
>> - if (value.data == NULL) {
>> - goto close;
>> - }
>> -
>> - value.len = path->len;
>> -
>> - rc = ngx_http_v2_parse_path(r, &value);
>> -
>> - if (rc != NGX_OK) {
>> - goto error;
>> - }
>> -
>> - for (header = ngx_http_v2_parse_headers; header->name.len; header++) {
>> - h = (ngx_table_elt_t **)
>> - ((char *) &parent->request->headers_in + header->offset);
>> -
>> - if (*h == NULL) {
>> - continue;
>> - }
>> -
>> - value.len = (*h)->value.len;
>> -
>> - value.data = ngx_pnalloc(pool, value.len + 1);
>> - if (value.data == NULL) {
>> - goto close;
>> - }
>> -
>> - ngx_memcpy(value.data, (*h)->value.data, value.len);
>> - value.data[value.len] = '\0';
>> -
>> - rc = ngx_http_v2_parse_header(r, header, &value);
>> -
>> - if (rc != NGX_OK) {
>> - goto error;
>> - }
>> - }
>> -
>> - fc->write->handler = ngx_http_v2_run_request_handler;
>> - ngx_post_event(fc->write, &ngx_posted_events);
>> -
>> - return stream;
>> -
>> -error:
>> -
>> - if (rc == NGX_ABORT) {
>> - /* header handler has already finalized request */
>> - ngx_http_run_posted_requests(fc);
>> - return NULL;
>> - }
>> -
>> - if (rc == NGX_DECLINED) {
>> - ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST);
>> - ngx_http_run_posted_requests(fc);
>> - return NULL;
>> - }
>> -
>> -close:
>> -
>> - ngx_http_v2_close_stream(stream, NGX_HTTP_INTERNAL_SERVER_ERROR);
>> -
>> - return NULL;
>> -
>> -rst_stream:
>> -
>> - if (ngx_http_v2_send_rst_stream(h2c, h2c->last_push,
>> - NGX_HTTP_INTERNAL_SERVER_ERROR)
>> - != NGX_OK)
>> - {
>> - h2c->connection->error = 1;
>> - }
>> -
>> - return NULL;
>> -}
>> -
>> -
>> static ngx_int_t
>> ngx_http_v2_send_settings(ngx_http_v2_connection_t *h2c)
>> {
>> @@ -3150,7 +2954,7 @@ ngx_http_v2_frame_handler(ngx_http_v2_co
>>
>>
>> static ngx_http_v2_stream_t *
>> -ngx_http_v2_create_stream(ngx_http_v2_connection_t *h2c, ngx_uint_t push)
>> +ngx_http_v2_create_stream(ngx_http_v2_connection_t *h2c)
>> {
>> ngx_log_t *log;
>> ngx_event_t *rev, *wev;
>> @@ -3205,13 +3009,7 @@ ngx_http_v2_create_stream(ngx_http_v2_co
>> ngx_memcpy(log, h2c->connection->log, sizeof(ngx_log_t));
>>
>> log->data = ctx;
>> -
>> - if (push) {
>> - log->action = "processing pushed request headers";
>> -
>> - } else {
>> - log->action = "reading client request headers";
>> - }
>> + log->action = "reading client request headers";
>>
>> ngx_memzero(rev, sizeof(ngx_event_t));
>>
>> @@ -3283,12 +3081,7 @@ ngx_http_v2_create_stream(ngx_http_v2_co
>> stream->send_window = h2c->init_window;
>> stream->recv_window = h2scf->preread_size;
>>
>> - if (push) {
>> - h2c->pushing++;
>> -
>> - } else {
>> - h2c->processing++;
>> - }
>> + h2c->processing++;
>>
>> h2c->priority_limit += h2scf->concurrent_streams;
>>
>> @@ -3711,45 +3504,40 @@ ngx_http_v2_parse_scheme(ngx_http_reques
>> static ngx_int_t
>> ngx_http_v2_parse_authority(ngx_http_request_t *r, ngx_str_t *value)
>> {
>> - return ngx_http_v2_parse_header(r, &ngx_http_v2_parse_headers[0], value);
>> -}
>> -
>> -
>> -static ngx_int_t
>> -ngx_http_v2_parse_header(ngx_http_request_t *r,
>> - ngx_http_v2_parse_header_t *header, ngx_str_t *value)
>> -{
>> ngx_table_elt_t *h;
>> + ngx_http_header_t *hh;
>> ngx_http_core_main_conf_t *cmcf;
>>
>> + static ngx_str_t host = ngx_string("host");
>> +
>> h = ngx_list_push(&r->headers_in.headers);
>> if (h == NULL) {
>> return NGX_ERROR;
>> }
>>
>> - h->key.len = header->name.len;
>> - h->key.data = header->name.data;
>> - h->lowcase_key = header->name.data;
>> -
>> - if (header->hh == NULL) {
>> - header->hash = ngx_hash_key(header->name.data, header->name.len);
>> -
>> - cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module);
>> -
>> - header->hh = ngx_hash_find(&cmcf->headers_in_hash, header->hash,
>> - h->lowcase_key, h->key.len);
>> - if (header->hh == NULL) {
>> - return NGX_ERROR;
>> - }
>> - }
>> -
>> - h->hash = header->hash;
>> + h->key.len = host.len;
>> + h->key.data = host.data;
>> + h->lowcase_key = host.data;
>> +
>> + h->hash = ngx_hash(ngx_hash(ngx_hash('h', 'o'), 's'), 't');
>>
>
> For the record: order of h->key/h->lowcase_key/h->hash assignments
> differs from the one used before 7207:3d2b0b02bd3d, but this is
> not important, and I agree that the new order is better.

Yep, it uses to preserve the order, not revert to pre-3d2b0b02bd3d.
Well, it can be rearranged to be consistent with the current order
of assignments in ngx_http_v2_construct_cookie_header():

diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -3509,15 +3509,16 @@ ngx_http_v2_parse_authority(ngx_http_req
return NGX_ERROR;
}

+ h->hash = ngx_hash(ngx_hash(ngx_hash('h', 'o'), 's'), 't');
+
h->key.len = host.len;
h->key.data = host.data;
- h->lowcase_key = host.data;
-
- h->hash = ngx_hash(ngx_hash(ngx_hash('h', 'o'), 's'), 't');

h->value.len = value->len;
h->value.data = value->data;

+ h->lowcase_key = host.data;
+
cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module);

hh = ngx_hash_find(&cmcf->headers_in_hash, h->hash,

(just in case, not applied)

>
>> h->value.len = value->len;
>> h->value.data = value->data;
>>
>> - if (header->hh->handler(r, h, header->hh->offset) != NGX_OK) {
>> - /* header handler has already finalized request */
>> + cmcf = ngx_http_get_module_main_conf(r, ngx_http_core_module);
>> +
>> + hh = ngx_hash_find(&cmcf->headers_in_hash, h->hash,
>> + h->lowcase_key, h->key.len);
>> +
>> + if (hh == NULL) {
>> + return NGX_ERROR;
>> + }
>> +
>> + if (hh->handler(r, h, hh->offset) != NGX_OK) {
>> + /*
>> + * request has been finalized already
>> + * in ngx_http_process_host()
>> + */
>> return NGX_ABORT;
>> }
>>

[..]

>> diff --git a/src/http/v2/ngx_http_v2_module.c b/src/http/v2/ngx_http_v2_module.c
>> --- a/src/http/v2/ngx_http_v2_module.c
>> +++ b/src/http/v2/ngx_http_v2_module.c
>> @@ -27,8 +27,6 @@ static void *ngx_http_v2_create_loc_conf
>> static char *ngx_http_v2_merge_loc_conf(ngx_conf_t *cf, void *parent,
>> void *child);
>>
>> -static char *ngx_http_v2_push(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
>> -
>> static char *ngx_http_v2_recv_buffer_size(ngx_conf_t *cf, void *post,
>> void *data);
>> static char *ngx_http_v2_pool_size(ngx_conf_t *cf, void *post, void *data);
>> @@ -38,6 +36,8 @@ static char *ngx_http_v2_streams_index_m
>> static char *ngx_http_v2_chunk_size(ngx_conf_t *cf, void *post, void *data);
>> static char *ngx_http_v2_obsolete(ngx_conf_t *cf, ngx_command_t *cmd,
>> void *conf);
>> +static char *ngx_http_v2_obsolete_push(ngx_conf_t *cf, ngx_command_t *cmd,
>> + void *conf);
>>
>>
>> static ngx_conf_deprecated_t ngx_http_v2_recv_timeout_deprecated = {
>> @@ -105,9 +105,9 @@ static ngx_command_t ngx_http_v2_comman
>>
>> { ngx_string("http2_max_concurrent_pushes"),
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
>> - ngx_conf_set_num_slot,
>> - NGX_HTTP_SRV_CONF_OFFSET,
>> - offsetof(ngx_http_v2_srv_conf_t, concurrent_pushes),
>> + ngx_http_v2_obsolete_push,
>> + 0,
>> + 0,
>> NULL },
>>
>> { ngx_string("http2_max_requests"),
>> @@ -168,15 +168,15 @@ static ngx_command_t ngx_http_v2_comman
>>
>> { ngx_string("http2_push_preload"),
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_FLAG,
>> - ngx_conf_set_flag_slot,
>> - NGX_HTTP_LOC_CONF_OFFSET,
>> - offsetof(ngx_http_v2_loc_conf_t, push_preload),
>> + ngx_http_v2_obsolete_push,
>> + 0,
>> + 0,
>> NULL },
>>
>> { ngx_string("http2_push"),
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
>> - ngx_http_v2_push,
>> - NGX_HTTP_LOC_CONF_OFFSET,
>> + ngx_http_v2_obsolete_push,
>> + 0,
>> 0,
>> NULL },
>>
>
> Using ngx_http_v2_obsolete() might be better, see below.

Agree, it doesn't look nice to deserve a separate function.

>
>> @@ -326,7 +326,6 @@ ngx_http_v2_create_srv_conf(ngx_conf_t *
>> h2scf->pool_size = NGX_CONF_UNSET_SIZE;
>>
>> h2scf->concurrent_streams = NGX_CONF_UNSET_UINT;
>> - h2scf->concurrent_pushes = NGX_CONF_UNSET_UINT;
>>
>> h2scf->preread_size = NGX_CONF_UNSET_SIZE;
>>
>> @@ -348,8 +347,6 @@ ngx_http_v2_merge_srv_conf(ngx_conf_t *c
>>
>> ngx_conf_merge_uint_value(conf->concurrent_streams,
>> prev->concurrent_streams, 128);
>> - ngx_conf_merge_uint_value(conf->concurrent_pushes,
>> - prev->concurrent_pushes, 10);
>>
>> ngx_conf_merge_size_value(conf->preread_size, prev->preread_size, 65536);
>>
>> @@ -370,17 +367,8 @@ ngx_http_v2_create_loc_conf(ngx_conf_t *
>> return NULL;
>> }
>>
>> - /*
>> - * set by ngx_pcalloc():
>> - *
>> - * h2lcf->pushes = NULL;
>> - */
>> -
>> h2lcf->chunk_size = NGX_CONF_UNSET_SIZE;
>>
>> - h2lcf->push_preload = NGX_CONF_UNSET;
>> - h2lcf->push = NGX_CONF_UNSET;
>> -
>> return h2lcf;
>> }
>>
>> @@ -393,72 +381,6 @@ ngx_http_v2_merge_loc_conf(ngx_conf_t *c
>>
>> ngx_conf_merge_size_value(conf->chunk_size, prev->chunk_size, 8 * 1024);
>>
>> - ngx_conf_merge_value(conf->push, prev->push, 1);
>> -
>> - if (conf->push && conf->pushes == NULL) {
>> - conf->pushes = prev->pushes;
>> - }
>> -
>> - ngx_conf_merge_value(conf->push_preload, prev->push_preload, 0);
>> -
>> - return NGX_CONF_OK;
>> -}
>> -
>> -
>> -static char *
>> -ngx_http_v2_push(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
>> -{
>> - ngx_http_v2_loc_conf_t *h2lcf = conf;
>> -
>> - ngx_str_t *value;
>> - ngx_http_complex_value_t *cv;
>> - ngx_http_compile_complex_value_t ccv;
>> -
>> - value = cf->args->elts;
>> -
>> - if (ngx_strcmp(value[1].data, "off") == 0) {
>> -
>> - if (h2lcf->pushes) {
>> - return "\"off\" parameter cannot be used with URI";
>> - }
>> -
>> - if (h2lcf->push == 0) {
>> - return "is duplicate";
>> - }
>> -
>> - h2lcf->push = 0;
>> - return NGX_CONF_OK;
>> - }
>> -
>> - if (h2lcf->push == 0) {
>> - return "URI cannot be used with \"off\" parameter";
>> - }
>> -
>> - h2lcf->push = 1;
>> -
>> - if (h2lcf->pushes == NULL) {
>> - h2lcf->pushes = ngx_array_create(cf->pool, 1,
>> - sizeof(ngx_http_complex_value_t));
>> - if (h2lcf->pushes == NULL) {
>> - return NGX_CONF_ERROR;
>> - }
>> - }
>> -
>> - cv = ngx_array_push(h2lcf->pushes);
>> - if (cv == NULL) {
>> - return NGX_CONF_ERROR;
>> - }
>> -
>> - ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
>> -
>> - ccv.cf = cf;
>> - ccv.value = &value[1];
>> - ccv.complex_value = cv;
>> -
>> - if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
>> - return NGX_CONF_ERROR;
>> - }
>> -
>> return NGX_CONF_OK;
>> }
>>
>> @@ -569,3 +491,14 @@ ngx_http_v2_obsolete(ngx_conf_t *cf, ngx
>>
>> return NGX_CONF_OK;
>> }
>> +
>> +
>> +static char *
>> +ngx_http_v2_obsolete_push(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
>> +{
>> + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
>> + "HTTP/2 server push was removed, "
>> + "the \"%V\" directive is obsolete", &cmd->name);
>
> I would rather use something like:
>
> ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> "the \"%V\" directive is obsolete, ignored",
> &cmd->name);

Agree, shorter is better.

>
> And it might make sense to update the ngx_http_v2_obsolete()
> function to do this if no cmd->post is provided.
>
> diff --git a/src/http/v2/ngx_http_v2_module.c b/src/http/v2/ngx_http_v2_module.c
> --- a/src/http/v2/ngx_http_v2_module.c
> +++ b/src/http/v2/ngx_http_v2_module.c
> @@ -36,8 +36,6 @@ static char *ngx_http_v2_streams_index_m
> static char *ngx_http_v2_chunk_size(ngx_conf_t *cf, void *post, void *data);
> static char *ngx_http_v2_obsolete(ngx_conf_t *cf, ngx_command_t *cmd,
> void *conf);
> -static char *ngx_http_v2_obsolete_push(ngx_conf_t *cf, ngx_command_t *cmd,
> - void *conf);
>
>
> static ngx_conf_deprecated_t ngx_http_v2_recv_timeout_deprecated = {
> @@ -105,7 +103,7 @@ static ngx_command_t ngx_http_v2_comman
>
> { ngx_string("http2_max_concurrent_pushes"),
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> - ngx_http_v2_obsolete_push,
> + ngx_http_v2_obsolete,
> 0,
> 0,
> NULL },
> @@ -168,14 +166,14 @@ static ngx_command_t ngx_http_v2_comman
>
> { ngx_string("http2_push_preload"),
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_FLAG,
> - ngx_http_v2_obsolete_push,
> + ngx_http_v2_obsolete,
> 0,
> 0,
> NULL },
>
> { ngx_string("http2_push"),
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> - ngx_http_v2_obsolete_push,
> + ngx_http_v2_obsolete,
> 0,
> 0,
> NULL },
> @@ -484,21 +482,17 @@ ngx_http_v2_obsolete(ngx_conf_t *cf, ngx
> {
> ngx_conf_deprecated_t *d = cmd->post;
>
> - ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> - "the \"%s\" directive is obsolete, "
> - "use the \"%s\" directive instead",
> - d->old_name, d->new_name);
> + if (d) {
> + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> + "the \"%s\" directive is obsolete, "
> + "use the \"%s\" directive instead",
> + d->old_name, d->new_name);
> +
> + } else {
> + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> + "the \"%V\" directive is obsolete, ignored",
> + &cmd->name);
> + }
>
> return NGX_CONF_OK;
> }
> -
> -
> -static char *
> -ngx_http_v2_obsolete_push(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> -{
> - ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> - "HTTP/2 server push was removed, "
> - "the \"%V\" directive is obsolete", &cmd->name);
> -
> - return NGX_CONF_OK;
> -}
>

Applied, thanks.
Updated patch attached.

>
>> +
>> + return NGX_CONF_OK;
>> +}
>> diff --git a/src/http/v2/ngx_http_v2_module.h b/src/http/v2/ngx_http_v2_module.h
>> --- a/src/http/v2/ngx_http_v2_module.h
>> +++ b/src/http/v2/ngx_http_v2_module.h
>> @@ -22,11 +22,6 @@ typedef struct {
>>
>> typedef struct {
>> size_t chunk_size;
>> -
>> - ngx_flag_t push_preload;
>> -
>> - ngx_flag_t push;
>> - ngx_array_t *pushes;
>> } ngx_http_v2_loc_conf_t;
>>
>>
>
> Otherwise looks good.
>


--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel