From 2ae788eddd4b842d3e6e5521c87e1c3239e188df Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 8 Nov 2013 00:12:39 +0900 Subject: [PATCH] Replace nghttp2_set_option with nghttp2_session_{client,server}_new2 nghttp2_session_client_new2 and nghttp2_session_server_new2 take additional parameters which specifies session options. nghttp2_set_option is somewhat crumsy because of type checking. Now we use nghttp2_opt_set, which specifies individual options with types. We changed the value of nghttp2_opt, so this change will require re-compile. --- lib/includes/nghttp2/nghttp2.h | 129 +++++++++++++++++++++------------ lib/nghttp2_session.c | 88 +++++++++++----------- src/nghttp.cc | 13 ++-- src/shrpx_http2_session.cc | 18 ++--- src/shrpx_http2_upstream.cc | 18 ++--- tests/nghttp2_session_test.c | 83 ++++++--------------- 6 files changed, 170 insertions(+), 179 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 2f85902e..131a964f 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -1236,14 +1236,6 @@ int nghttp2_session_server_new(nghttp2_session **session_ptr, const nghttp2_session_callbacks *callbacks, void *user_data); -/** - * @function - * - * Frees any resources allocated for |session|. If |session| is - * ``NULL``, this function does nothing. - */ -void nghttp2_session_del(nghttp2_session *session); - /** * @enum * @@ -1252,18 +1244,22 @@ void nghttp2_session_del(nghttp2_session *session); typedef enum { /** * This option prevents the library from sending WINDOW_UPDATE for a - * stream automatically. If this option is set, the application is - * responsible for sending WINDOW_UPDATE using - * `nghttp2_submit_window_update`. + * stream automatically. If this option is set to nonzero, the + * library won't send WINDOW_UPDATE for a stream and the application + * is responsible for sending WINDOW_UPDATE using + * `nghttp2_submit_window_update`. By default, this option is set to + * zero. */ NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE = 1, /** * This option prevents the library from sending WINDOW_UPDATE for a - * connection automatically. If this option is set, the application - * is responsible for sending WINDOW_UPDATE with stream ID 0 using - * `nghttp2_submit_window_update`. + * connection automatically. If this option is set to nonzero, the + * library won't send WINDOW_UPDATE for a connection and the + * application is responsible for sending WINDOW_UPDATE with stream + * ID 0 using `nghttp2_submit_window_update`. By default, this + * option is set to zero. */ - NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE = 2, + NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE = 1 << 1, /** * This option sets the SETTINGS_MAX_CONCURRENT_STREAMS value of * remote endpoint as if it is received in SETTINGS frame. Without @@ -1277,47 +1273,90 @@ typedef enum { * will be overwritten if the local endpoint receives * SETTINGS_MAX_CONCURRENT_STREAMS from the remote endpoint. */ - NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS = 3 + NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS = 1 << 2 } nghttp2_opt; +/** + * @struct + * + * Struct to store option values for nghttp2_session. + */ +typedef struct { + /** + * :enum:`NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE` + */ + uint8_t no_auto_stream_window_update; + /** + * :enum:`NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE` + */ + uint8_t no_auto_connection_window_update; + /** + * :enum:`NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS` + */ + uint32_t peer_max_concurrent_streams; +} nghttp2_opt_set; + /** * @function * - * Sets the configuration option for the |session|. The |optname| is - * one of :type:`nghttp2_opt`. The |optval| is the pointer to the - * option value and the |optlen| is the size of |*optval|. The - * required type of |optval| varies depending on the |optname|. See - * below. + * Like `nghttp2_session_client_new()`, but with additional options + * specified in the |opt_set|. The caller must set bitwise-OR of + * :enum:`nghttp2_opt` for given options. For example, if it + * specifies :enum:`NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE` and + * :enum:`NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS` in the |opt_set|, + * the |opt_set_mask| should be + * ``NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE | + * NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS``. * - * The following |optname| are supported: - * - * :enum:`NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE` - * The |optval| must be a pointer to ``int``. If the |*optval| is - * nonzero, the library will not send WINDOW_UPDATE for a stream - * automatically. Therefore, the application is responsible for - * sending WINDOW_UPDATE using - * `nghttp2_submit_window_update`. This option defaults to 0. - * - * :enum:`NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE` - * The |optval| must be a pointer to ``int``. If the |*optval| is - * nonzero, the library will not send WINDOW_UPDATE for connection - * automatically. Therefore, the application is responsible for - * sending WINDOW_UPDATE using - * `nghttp2_submit_window_update`. This option defaults to 0. - * - * :enum:`NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS` - * The |optval| must be a pointer to ``ssize_t``. It is an error - * if |*optval| is 0 or negative. + * If the |opt_set_mask| is 0, the |opt_set| could be ``NULL`` safely + * and the call is equivalent to `nghttp2_session_client_new()`. * * This function returns 0 if it succeeds, or one of the following * negative error codes: * - * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` - * The |optname| is not supported; or the |optval| and/or the - * |optlen| are invalid. + * :enum:`NGHTTP2_ERR_NOMEM` + * Out of memory. */ -int nghttp2_session_set_option(nghttp2_session *session, - int optname, void *optval, size_t optlen); +int nghttp2_session_client_new2(nghttp2_session **session_ptr, + const nghttp2_session_callbacks *callbacks, + void *user_data, + uint32_t opt_set_mask, + const nghttp2_opt_set *opt_set); + +/** + * @function + * + * Like `nghttp2_session_server_new()`, but with additional options + * specified in the |opt_set|. The caller must set bitwise-OR of + * :enum:`nghttp2_opt` for given options. For example, if it + * specifies :enum:`NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE` and + * :enum:`NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS` in the |opt_set|, + * the |opt_set_mask| should be + * ``NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE | + * NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS``. + * + * If the |opt_set_mask| is 0, the |opt_set| could be ``NULL`` safely + * and the call is equivalent to `nghttp2_session_server_new()`. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * :enum:`NGHTTP2_ERR_NOMEM` + * Out of memory. + */ +int nghttp2_session_server_new2(nghttp2_session **session_ptr, + const nghttp2_session_callbacks *callbacks, + void *user_data, + uint32_t opt_set_mask, + const nghttp2_opt_set *opt_set); + +/** + * @function + * + * Frees any resources allocated for |session|. If |session| is + * ``NULL``, this function does nothing. + */ +void nghttp2_session_del(nghttp2_session *session); /** * @function diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 4e609d0a..bae00a11 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -177,7 +177,9 @@ static void init_settings(uint32_t *settings) static int nghttp2_session_new(nghttp2_session **session_ptr, const nghttp2_session_callbacks *callbacks, void *user_data, - int server) + int server, + uint32_t opt_set_mask, + const nghttp2_opt_set *opt_set) { int r; nghttp2_hd_side side_deflate, side_inflate; @@ -193,6 +195,16 @@ static int nghttp2_session_new(nghttp2_session **session_ptr, (*session_ptr)->next_seq = 0; + if((opt_set_mask & NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE) && + opt_set->no_auto_stream_window_update) { + (*session_ptr)->opt_flags |= NGHTTP2_OPTMASK_NO_AUTO_STREAM_WINDOW_UPDATE; + } + if((opt_set_mask & NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE) && + opt_set->no_auto_connection_window_update) { + (*session_ptr)->opt_flags |= + NGHTTP2_OPTMASK_NO_AUTO_CONNECTION_WINDOW_UPDATE; + } + (*session_ptr)->remote_flow_control = 1; (*session_ptr)->local_flow_control = 1; (*session_ptr)->remote_window_size = NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE; @@ -254,6 +266,11 @@ static int nghttp2_session_new(nghttp2_session **session_ptr, init_settings((*session_ptr)->remote_settings); init_settings((*session_ptr)->local_settings); + if(opt_set_mask & NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS) { + (*session_ptr)->remote_settings[NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS] = + opt_set->peer_max_concurrent_streams; + } + (*session_ptr)->callbacks = *callbacks; (*session_ptr)->user_data = user_data; @@ -290,10 +307,21 @@ static int nghttp2_session_new(nghttp2_session **session_ptr, int nghttp2_session_client_new(nghttp2_session **session_ptr, const nghttp2_session_callbacks *callbacks, void *user_data) +{ + return nghttp2_session_client_new2(session_ptr, callbacks, user_data, + 0, NULL); +} + +int nghttp2_session_client_new2(nghttp2_session **session_ptr, + const nghttp2_session_callbacks *callbacks, + void *user_data, + uint32_t opt_set_mask, + const nghttp2_opt_set *opt_set) { int r; /* For client side session, header compression is disabled. */ - r = nghttp2_session_new(session_ptr, callbacks, user_data, 0); + r = nghttp2_session_new(session_ptr, callbacks, user_data, 0, + opt_set_mask, opt_set); if(r == 0) { /* IDs for use in client */ (*session_ptr)->next_stream_id = 1; @@ -305,10 +333,21 @@ int nghttp2_session_client_new(nghttp2_session **session_ptr, int nghttp2_session_server_new(nghttp2_session **session_ptr, const nghttp2_session_callbacks *callbacks, void *user_data) +{ + return nghttp2_session_server_new2(session_ptr, callbacks, user_data, + 0, NULL); +} + +int nghttp2_session_server_new2(nghttp2_session **session_ptr, + const nghttp2_session_callbacks *callbacks, + void *user_data, + uint32_t opt_set_mask, + const nghttp2_opt_set *opt_set) { int r; /* Enable header compression on server side. */ - r = nghttp2_session_new(session_ptr, callbacks, user_data, 1); + r = nghttp2_session_new(session_ptr, callbacks, user_data, 1, + opt_set_mask, opt_set); if(r == 0) { /* IDs for use in client */ (*session_ptr)->next_stream_id = 2; @@ -3626,49 +3665,6 @@ int32_t nghttp2_session_get_effective_local_window_size return session->local_window_size; } -int nghttp2_session_set_option(nghttp2_session *session, - int optname, void *optval, size_t optlen) -{ - switch(optname) { - case NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE: - case NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE: { - int flag; - if(optname == NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE) { - flag = NGHTTP2_OPTMASK_NO_AUTO_STREAM_WINDOW_UPDATE; - } else { - flag = NGHTTP2_OPTMASK_NO_AUTO_CONNECTION_WINDOW_UPDATE; - } - if(optlen == sizeof(int)) { - int intval = *(int*)optval; - if(intval) { - session->opt_flags |= flag; - } else { - session->opt_flags &= ~flag; - } - } else { - return NGHTTP2_ERR_INVALID_ARGUMENT; - } - break; - } - case NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS: { - ssize_t sszval; - if(optlen != sizeof(ssize_t)) { - return NGHTTP2_ERR_INVALID_ARGUMENT; - } - sszval = *(ssize_t*)optval; - if(sszval <= 0) { - return NGHTTP2_ERR_INVALID_ARGUMENT; - } - session->remote_settings[NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS] = - sszval; - break; - } - default: - return NGHTTP2_ERR_INVALID_ARGUMENT; - } - return 0; -} - int nghttp2_session_upgrade(nghttp2_session *session, const uint8_t *settings_payload, size_t settings_payloadlen, diff --git a/src/nghttp.cc b/src/nghttp.cc index 95251b6b..09ad2f4e 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -665,14 +665,11 @@ struct HttpClient { if(!need_upgrade()) { record_handshake_time(); } - rv = nghttp2_session_client_new(&session, callbacks, this); - if(rv != 0) { - return -1; - } - rv = nghttp2_session_set_option(session, - NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS, - &config.peer_max_concurrent_streams, - sizeof(config.peer_max_concurrent_streams)); + nghttp2_opt_set opt_set; + opt_set.peer_max_concurrent_streams = config.peer_max_concurrent_streams; + rv = nghttp2_session_client_new2(&session, callbacks, this, + NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS, + &opt_set); if(rv != 0) { return -1; } diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index d6f26348..7a7339e5 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -1148,21 +1148,19 @@ int Http2Session::on_connect() on_frame_recv_parse_error_callback; callbacks.on_unknown_frame_recv_callback = on_unknown_frame_recv_callback; - rv = nghttp2_session_client_new(&session_, &callbacks, this); + nghttp2_opt_set opt_set; + opt_set.no_auto_stream_window_update = 1; + opt_set.no_auto_connection_window_update = 1; + uint32_t opt_set_mask = + NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE | + NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE; + rv = nghttp2_session_client_new2(&session_, &callbacks, this, + opt_set_mask, &opt_set); if(rv != 0) { return -1; } - int val = 1; flow_control_ = true; - rv = nghttp2_session_set_option(session_, - NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE, - &val, sizeof(val)); - assert(rv == 0); - rv = nghttp2_session_set_option(session_, - NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE, - &val, sizeof(val)); - assert(rv == 0); nghttp2_settings_entry entry[3]; entry[0].settings_id = NGHTTP2_SETTINGS_ENABLE_PUSH; diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index c009197e..7a90c579 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -484,21 +484,19 @@ Http2Upstream::Http2Upstream(ClientHandler *handler) on_frame_recv_parse_error_callback; callbacks.on_unknown_frame_recv_callback = on_unknown_frame_recv_callback; + nghttp2_opt_set opt_set; + opt_set.no_auto_stream_window_update = 1; + opt_set.no_auto_connection_window_update = 1; + uint32_t opt_set_mask = + NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE | + NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE; int rv; - rv = nghttp2_session_server_new(&session_, &callbacks, this); + rv = nghttp2_session_server_new2(&session_, &callbacks, this, + opt_set_mask, &opt_set); assert(rv == 0); - int val = 1; flow_control_ = true; initial_window_size_ = (1 << get_config()->http2_upstream_window_bits) - 1; - rv = nghttp2_session_set_option(session_, - NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE, - &val, sizeof(val)); - assert(rv == 0); - rv = nghttp2_session_set_option(session_, - NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE, - &val, sizeof(val)); - assert(rv == 0); // TODO Maybe call from outside? nghttp2_settings_entry entry[2]; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 4d1d36cf..b5955daa 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -3605,73 +3605,36 @@ void test_nghttp2_session_set_option(void) { nghttp2_session* session; nghttp2_session_callbacks callbacks; - int intval; - char charval; - ssize_t sszval; + nghttp2_opt_set opt_set; + + opt_set.no_auto_stream_window_update = 1; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); - nghttp2_session_client_new(&session, &callbacks, NULL); - - intval = 1; - CU_ASSERT(0 == - nghttp2_session_set_option - (session, - NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE, - &intval, sizeof(intval))); + nghttp2_session_client_new2(&session, &callbacks, NULL, + NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE, + &opt_set); CU_ASSERT(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_STREAM_WINDOW_UPDATE); + CU_ASSERT(!(session->opt_flags & + NGHTTP2_OPTMASK_NO_AUTO_CONNECTION_WINDOW_UPDATE)); + nghttp2_session_del(session); - intval = 0; - CU_ASSERT(0 == - nghttp2_session_set_option - (session, - NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE, - &intval, sizeof(intval))); - CU_ASSERT((session->opt_flags & - NGHTTP2_OPTMASK_NO_AUTO_STREAM_WINDOW_UPDATE) == 0); - - CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == - nghttp2_session_set_option(session, 0, /* 0 is invalid optname */ - &intval, sizeof(intval))); - - charval = 1; - CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == - nghttp2_session_set_option - (session, - NGHTTP2_OPT_NO_AUTO_STREAM_WINDOW_UPDATE, - &charval, sizeof(charval))); - - intval = 1; - CU_ASSERT(0 == - nghttp2_session_set_option - (session, - NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE, - &intval, sizeof(intval))); + opt_set.no_auto_stream_window_update = 0; + opt_set.no_auto_connection_window_update = 1; + nghttp2_session_server_new2(&session, &callbacks, NULL, + NGHTTP2_OPT_NO_AUTO_CONNECTION_WINDOW_UPDATE, + &opt_set); + CU_ASSERT(!(session->opt_flags & + NGHTTP2_OPTMASK_NO_AUTO_STREAM_WINDOW_UPDATE)); CU_ASSERT(session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_CONNECTION_WINDOW_UPDATE); + nghttp2_session_del(session); - sszval = 100; - CU_ASSERT(0 == - nghttp2_session_set_option - (session, - NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS, - &sszval, sizeof(sszval))); - CU_ASSERT(sszval == - (ssize_t)session-> + opt_set.peer_max_concurrent_streams = 100; + nghttp2_session_client_new2(&session, &callbacks, NULL, + NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS, + &opt_set); + CU_ASSERT(100 == + session-> remote_settings[NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS]); - - sszval = 0; - CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == - nghttp2_session_set_option - (session, - NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS, - &sszval, sizeof(sszval))); - - charval = 100; - CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == - nghttp2_session_set_option - (session, - NGHTTP2_OPT_PEER_MAX_CONCURRENT_STREAMS, - &charval, sizeof(charval))); - nghttp2_session_del(session); }