From a53f0f0a17ecf86dbb09f6e7a0d397f5cfda2431 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 14 Feb 2016 18:47:24 +0900 Subject: [PATCH 1/4] nghttpx: Refactor DownstreamAddrGroup and router API --- src/shrpx.cc | 12 ++++++------ src/shrpx_config.cc | 27 ++++++--------------------- src/shrpx_config.h | 9 +++------ src/shrpx_config_test.cc | 2 +- 4 files changed, 16 insertions(+), 34 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index 87bd9c28..24290891 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -2109,14 +2109,14 @@ void process_options( addr.host = ImmutableString::from_lit(DEFAULT_DOWNSTREAM_HOST); addr.port = DEFAULT_DOWNSTREAM_PORT; - DownstreamAddrGroup g("/"); + DownstreamAddrGroup g(StringRef::from_lit("/")); g.addrs.push_back(std::move(addr)); - mod_config()->router.add_route(g.pattern.get(), 1, addr_groups.size()); + mod_config()->router.add_route(g.pattern.c_str(), 1, addr_groups.size()); addr_groups.push_back(std::move(g)); } else if (get_config()->http2_proxy || get_config()->client_proxy) { // We don't support host mapping in these cases. Move all // non-catch-all patterns to catch-all pattern. - DownstreamAddrGroup catch_all("/"); + DownstreamAddrGroup catch_all(StringRef::from_lit("/")); for (auto &g : addr_groups) { std::move(std::begin(g.addrs), std::end(g.addrs), std::back_inserter(catch_all.addrs)); @@ -2124,7 +2124,7 @@ void process_options( std::vector().swap(addr_groups); // maybe not necessary? mod_config()->router = Router(); - mod_config()->router.add_route(catch_all.pattern.get(), 1, + mod_config()->router.add_route(catch_all.pattern.c_str(), 1, addr_groups.size()); addr_groups.push_back(std::move(catch_all)); } @@ -2136,11 +2136,11 @@ void process_options( ssize_t catch_all_group = -1; for (size_t i = 0; i < addr_groups.size(); ++i) { auto &g = addr_groups[i]; - if (util::streq(g.pattern.get(), "/")) { + if (g.pattern == "/") { catch_all_group = i; } if (LOG_ENABLED(INFO)) { - LOG(INFO) << "Host-path pattern: group " << i << ": '" << g.pattern.get() + LOG(INFO) << "Host-path pattern: group " << i << ": '" << g.pattern << "'"; for (auto &addr : g.addrs) { LOG(INFO) << "group " << i << " -> " << addr.host.c_str() diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index 489885cf..23b480b3 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -78,21 +78,6 @@ TicketKeys::~TicketKeys() { } } -DownstreamAddrGroup::DownstreamAddrGroup(const DownstreamAddrGroup &other) - : pattern(strcopy(other.pattern)), addrs(other.addrs) {} - -DownstreamAddrGroup &DownstreamAddrGroup:: -operator=(const DownstreamAddrGroup &other) { - if (this == &other) { - return *this; - } - - pattern = strcopy(other.pattern); - addrs = other.addrs; - - return *this; -} - namespace { int split_host_port(char *host, size_t hostlen, uint16_t *port_ptr, const char *hostport, size_t hostportlen) { @@ -612,7 +597,7 @@ void parse_mapping(const DownstreamAddr &addr, const char *src) { pattern += http2::normalize_path(slash, raw_pattern.second); } for (auto &g : addr_groups) { - if (g.pattern.get() == pattern) { + if (g.pattern == pattern) { g.addrs.push_back(addr); done = true; break; @@ -621,10 +606,10 @@ void parse_mapping(const DownstreamAddr &addr, const char *src) { if (done) { continue; } - DownstreamAddrGroup g(pattern); + DownstreamAddrGroup g(StringRef{pattern}); g.addrs.push_back(addr); - mod_config()->router.add_route(g.pattern.get(), strlen(g.pattern.get()), + mod_config()->router.add_route(g.pattern.c_str(), g.pattern.size(), addr_groups.size()); addr_groups.push_back(std::move(g)); @@ -2529,7 +2514,7 @@ match_downstream_addr_group_host(const Router &router, const std::string &host, if (group != -1) { if (LOG_ENABLED(INFO)) { LOG(INFO) << "Found pattern with query " << host - << ", matched pattern=" << groups[group].pattern.get(); + << ", matched pattern=" << groups[group].pattern; } return group; } @@ -2546,7 +2531,7 @@ match_downstream_addr_group_host(const Router &router, const std::string &host, if (LOG_ENABLED(INFO)) { LOG(INFO) << "Found pattern with query " << host << std::string(path, pathlen) - << ", matched pattern=" << groups[group].pattern.get(); + << ", matched pattern=" << groups[group].pattern; } return group; } @@ -2555,7 +2540,7 @@ match_downstream_addr_group_host(const Router &router, const std::string &host, if (group != -1) { if (LOG_ENABLED(INFO)) { LOG(INFO) << "Found pattern with query " << std::string(path, pathlen) - << ", matched pattern=" << groups[group].pattern.get(); + << ", matched pattern=" << groups[group].pattern; } return group; } diff --git a/src/shrpx_config.h b/src/shrpx_config.h index c460cbc8..e024317a 100644 --- a/src/shrpx_config.h +++ b/src/shrpx_config.h @@ -301,13 +301,10 @@ struct DownstreamAddr { }; struct DownstreamAddrGroup { - DownstreamAddrGroup(const std::string &pattern) : pattern(strcopy(pattern)) {} - DownstreamAddrGroup(const DownstreamAddrGroup &other); - DownstreamAddrGroup(DownstreamAddrGroup &&) = default; - DownstreamAddrGroup &operator=(const DownstreamAddrGroup &other); - DownstreamAddrGroup &operator=(DownstreamAddrGroup &&) = default; + DownstreamAddrGroup(const StringRef &pattern) + : pattern(pattern.c_str(), pattern.size()) {} - std::unique_ptr pattern; + ImmutableString pattern; std::vector addrs; }; diff --git a/src/shrpx_config_test.cc b/src/shrpx_config_test.cc index eda67e85..fa8c1391 100644 --- a/src/shrpx_config_test.cc +++ b/src/shrpx_config_test.cc @@ -256,7 +256,7 @@ void test_shrpx_config_match_downstream_addr_group(void) { for (size_t i = 0; i < groups.size(); ++i) { auto &g = groups[i]; - router.add_route(g.pattern.get(), strlen(g.pattern.get()), i); + router.add_route(g.pattern.c_str(), g.pattern.size(), i); } CU_ASSERT(0 == match_downstream_addr_group(router, "nghttp2.org", "/", groups, From 2d273f82375b527b081bf1e3b2fd60a6024c30de Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 14 Feb 2016 18:55:53 +0900 Subject: [PATCH 2/4] nghttpx: Use StringRef for pattern paramter in Router::add_route --- src/shrpx.cc | 4 ++-- src/shrpx_config.cc | 3 +-- src/shrpx_config_test.cc | 2 +- src/shrpx_router.cc | 12 ++++++------ src/shrpx_router.h | 4 ++-- 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index 24290891..ee05263b 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -2111,7 +2111,7 @@ void process_options( DownstreamAddrGroup g(StringRef::from_lit("/")); g.addrs.push_back(std::move(addr)); - mod_config()->router.add_route(g.pattern.c_str(), 1, addr_groups.size()); + mod_config()->router.add_route(StringRef{g.pattern}, addr_groups.size()); addr_groups.push_back(std::move(g)); } else if (get_config()->http2_proxy || get_config()->client_proxy) { // We don't support host mapping in these cases. Move all @@ -2124,7 +2124,7 @@ void process_options( std::vector().swap(addr_groups); // maybe not necessary? mod_config()->router = Router(); - mod_config()->router.add_route(catch_all.pattern.c_str(), 1, + mod_config()->router.add_route(StringRef{catch_all.pattern}, addr_groups.size()); addr_groups.push_back(std::move(catch_all)); } diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index 23b480b3..9dd5c444 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -609,8 +609,7 @@ void parse_mapping(const DownstreamAddr &addr, const char *src) { DownstreamAddrGroup g(StringRef{pattern}); g.addrs.push_back(addr); - mod_config()->router.add_route(g.pattern.c_str(), g.pattern.size(), - addr_groups.size()); + mod_config()->router.add_route(StringRef{g.pattern}, addr_groups.size()); addr_groups.push_back(std::move(g)); } diff --git a/src/shrpx_config_test.cc b/src/shrpx_config_test.cc index fa8c1391..53b1bc30 100644 --- a/src/shrpx_config_test.cc +++ b/src/shrpx_config_test.cc @@ -256,7 +256,7 @@ void test_shrpx_config_match_downstream_addr_group(void) { for (size_t i = 0; i < groups.size(); ++i) { auto &g = groups[i]; - router.add_route(g.pattern.c_str(), g.pattern.size(), i); + router.add_route(StringRef{g.pattern}, i); } CU_ASSERT(0 == match_downstream_addr_group(router, "nghttp2.org", "/", groups, diff --git a/src/shrpx_router.cc b/src/shrpx_router.cc index e0404252..4324748a 100644 --- a/src/shrpx_router.cc +++ b/src/shrpx_router.cc @@ -66,21 +66,21 @@ void Router::add_node(RNode *node, const char *pattern, size_t patlen, add_next_node(node, std::move(new_node)); } -bool Router::add_route(const char *pattern, size_t patlen, size_t index) { +bool Router::add_route(const StringRef &pattern, size_t index) { auto node = &root_; size_t i = 0; for (;;) { auto next_node = find_next_node(node, pattern[i]); if (next_node == nullptr) { - add_node(node, pattern + i, patlen - i, index); + add_node(node, pattern.c_str() + i, pattern.size() - i, index); return true; } node = next_node; - auto slen = patlen - i; - auto s = pattern + i; + auto slen = pattern.size() - i; + auto s = pattern.c_str() + i; auto n = std::min(node->len, slen); size_t j; for (j = 0; j < n && node->s[j] == s[j]; ++j) @@ -125,8 +125,8 @@ bool Router::add_route(const char *pattern, size_t patlen, size_t index) { i += j; - assert(patlen > i); - add_node(node, pattern + i, patlen - i, index); + assert(pattern.size() > i); + add_node(node, pattern.c_str() + i, pattern.size() - i, index); return true; } diff --git a/src/shrpx_router.h b/src/shrpx_router.h index 07aadf69..e4d9a465 100644 --- a/src/shrpx_router.h +++ b/src/shrpx_router.h @@ -55,8 +55,8 @@ struct RNode { class Router { public: Router(); - // Adds route |pattern| of size |patlen| with its |index|. - bool add_route(const char *pattern, size_t patlen, size_t index); + // Adds route |pattern| with its |index|. + bool add_route(const StringRef &pattern, size_t index); // Returns the matched index of pattern. -1 if there is no match. ssize_t match(const std::string &host, const char *path, size_t pathlen) const; From 93eabc642b9732a5172b5b2eaf104320d46fa68d Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 14 Feb 2016 19:07:22 +0900 Subject: [PATCH 3/4] nghttpx: Use StringRef for parameter in Router::match --- src/shrpx_config.cc | 32 ++++++++++++++------------------ src/shrpx_router.cc | 5 ++--- src/shrpx_router.h | 3 +-- src/template.h | 3 +++ 4 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index 9dd5c444..a5d9f534 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -2503,13 +2503,11 @@ int int_syslog_facility(const char *strfacility) { } namespace { -size_t -match_downstream_addr_group_host(const Router &router, const std::string &host, - const char *path, size_t pathlen, - const std::vector &groups, - size_t catch_all) { - if (pathlen == 0 || *path != '/') { - auto group = router.match(host, "/", 1); +size_t match_downstream_addr_group_host( + const Router &router, const std::string &host, const StringRef &path, + const std::vector &groups, size_t catch_all) { + if (path.empty() || path[0] != '/') { + auto group = router.match(host, StringRef::from_lit("/")); if (group != -1) { if (LOG_ENABLED(INFO)) { LOG(INFO) << "Found pattern with query " << host @@ -2522,23 +2520,22 @@ match_downstream_addr_group_host(const Router &router, const std::string &host, if (LOG_ENABLED(INFO)) { LOG(INFO) << "Perform mapping selection, using host=" << host - << ", path=" << std::string(path, pathlen); + << ", path=" << path; } - auto group = router.match(host, path, pathlen); + auto group = router.match(host, path); if (group != -1) { if (LOG_ENABLED(INFO)) { - LOG(INFO) << "Found pattern with query " << host - << std::string(path, pathlen) + LOG(INFO) << "Found pattern with query " << host << path << ", matched pattern=" << groups[group].pattern; } return group; } - group = router.match("", path, pathlen); + group = router.match("", path); if (group != -1) { if (LOG_ENABLED(INFO)) { - LOG(INFO) << "Found pattern with query " << std::string(path, pathlen) + LOG(INFO) << "Found pattern with query " << path << ", matched pattern=" << groups[group].pattern; } return group; @@ -2565,12 +2562,11 @@ match_downstream_addr_group(const Router &router, const std::string &hostport, auto fragment = std::find(std::begin(raw_path), std::end(raw_path), '#'); auto query = std::find(std::begin(raw_path), fragment, '?'); - auto path = raw_path.c_str(); - auto pathlen = query - std::begin(raw_path); + auto path = StringRef{std::begin(raw_path), query}; if (hostport.empty()) { - return match_downstream_addr_group_host(router, hostport, path, pathlen, - groups, catch_all); + return match_downstream_addr_group_host(router, hostport, path, groups, + catch_all); } std::string host; @@ -2593,7 +2589,7 @@ match_downstream_addr_group(const Router &router, const std::string &hostport, } util::inp_strlower(host); - return match_downstream_addr_group_host(router, host, path, pathlen, groups, + return match_downstream_addr_group_host(router, host, path, groups, catch_all); } diff --git a/src/shrpx_router.cc b/src/shrpx_router.cc index 4324748a..2975c3e9 100644 --- a/src/shrpx_router.cc +++ b/src/shrpx_router.cc @@ -259,8 +259,7 @@ const RNode *match_partial(const RNode *node, size_t offset, const char *first, } } // namespace -ssize_t Router::match(const std::string &host, const char *path, - size_t pathlen) const { +ssize_t Router::match(const std::string &host, const StringRef &path) const { const RNode *node; size_t offset; @@ -270,7 +269,7 @@ ssize_t Router::match(const std::string &host, const char *path, return -1; } - node = match_partial(node, offset, path, path + pathlen); + node = match_partial(node, offset, std::begin(path), std::end(path)); if (node == nullptr || node == &root_) { return -1; } diff --git a/src/shrpx_router.h b/src/shrpx_router.h index e4d9a465..5f70b212 100644 --- a/src/shrpx_router.h +++ b/src/shrpx_router.h @@ -58,8 +58,7 @@ public: // Adds route |pattern| with its |index|. bool add_route(const StringRef &pattern, size_t index); // Returns the matched index of pattern. -1 if there is no match. - ssize_t match(const std::string &host, const char *path, - size_t pathlen) const; + ssize_t match(const std::string &host, const StringRef &path) const; void add_node(RNode *node, const char *pattern, size_t patlen, size_t index); diff --git a/src/template.h b/src/template.h index 3061ad52..253ea215 100644 --- a/src/template.h +++ b/src/template.h @@ -397,6 +397,9 @@ public: : base(reinterpret_cast(s)), len(n) {} template StringRef(InputIt first, InputIt last) + : base(&*first), len(std::distance(first, last)) {} + template + StringRef(InputIt *first, InputIt *last) : base(first), len(std::distance(first, last)) {} template constexpr static StringRef from_lit(const CharT(&s)[N]) { From 49fa914db5cbf74708e025f9aeae74f9a79f2ab1 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sun, 14 Feb 2016 20:48:06 +0900 Subject: [PATCH 4/4] nghttpx: Use StringRef for string parameters in match_downstream_addr_group --- src/shrpx_client_handler.cc | 15 +++++++++------ src/shrpx_config.cc | 12 +++++------- src/shrpx_config.h | 2 +- src/shrpx_router.cc | 5 ++--- src/shrpx_router.h | 2 +- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index fa814dd9..4d063647 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -681,16 +681,19 @@ ClientHandler::get_downstream_connection(Downstream *downstream) { } else { auto &router = get_config()->router; if (!req.authority.empty()) { - group = match_downstream_addr_group(router, req.authority, req.path, - groups, catch_all); + group = + match_downstream_addr_group(router, StringRef{req.authority}, + StringRef{req.path}, groups, catch_all); } else { auto h = req.fs.header(http2::HD_HOST); if (h) { - group = match_downstream_addr_group(router, h->value, req.path, groups, - catch_all); + group = + match_downstream_addr_group(router, StringRef{h->value}, + StringRef{req.path}, groups, catch_all); } else { - group = match_downstream_addr_group(router, "", req.path, groups, - catch_all); + group = + match_downstream_addr_group(router, StringRef::from_lit(""), + StringRef{req.path}, groups, catch_all); } } } diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index a5d9f534..5e6236c3 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -2504,7 +2504,7 @@ int int_syslog_facility(const char *strfacility) { namespace { size_t match_downstream_addr_group_host( - const Router &router, const std::string &host, const StringRef &path, + const Router &router, const StringRef &host, const StringRef &path, const std::vector &groups, size_t catch_all) { if (path.empty() || path[0] != '/') { auto group = router.match(host, StringRef::from_lit("/")); @@ -2548,11 +2548,9 @@ size_t match_downstream_addr_group_host( } } // namespace -size_t -match_downstream_addr_group(const Router &router, const std::string &hostport, - const std::string &raw_path, - const std::vector &groups, - size_t catch_all) { +size_t match_downstream_addr_group( + const Router &router, const StringRef &hostport, const StringRef &raw_path, + const std::vector &groups, size_t catch_all) { if (std::find(std::begin(hostport), std::end(hostport), '/') != std::end(hostport)) { // We use '/' specially, and if '/' is included in host, it breaks @@ -2589,7 +2587,7 @@ match_downstream_addr_group(const Router &router, const std::string &hostport, } util::inp_strlower(host); - return match_downstream_addr_group_host(router, host, path, groups, + return match_downstream_addr_group_host(router, StringRef{host}, path, groups, catch_all); } diff --git a/src/shrpx_config.h b/src/shrpx_config.h index e024317a..b81f8e1d 100644 --- a/src/shrpx_config.h +++ b/src/shrpx_config.h @@ -652,7 +652,7 @@ read_tls_ticket_key_file(const std::vector &files, // group. The catch-all group index is given in |catch_all|. All // patterns are given in |groups|. size_t match_downstream_addr_group( - const Router &router, const std::string &hostport, const std::string &path, + const Router &router, const StringRef &hostport, const StringRef &path, const std::vector &groups, size_t catch_all); } // namespace shrpx diff --git a/src/shrpx_router.cc b/src/shrpx_router.cc index 2975c3e9..4c88283f 100644 --- a/src/shrpx_router.cc +++ b/src/shrpx_router.cc @@ -259,12 +259,11 @@ const RNode *match_partial(const RNode *node, size_t offset, const char *first, } } // namespace -ssize_t Router::match(const std::string &host, const StringRef &path) const { +ssize_t Router::match(const StringRef &host, const StringRef &path) const { const RNode *node; size_t offset; - node = - match_complete(&offset, &root_, host.c_str(), host.c_str() + host.size()); + node = match_complete(&offset, &root_, std::begin(host), std::end(host)); if (node == nullptr) { return -1; } diff --git a/src/shrpx_router.h b/src/shrpx_router.h index 5f70b212..ece0e276 100644 --- a/src/shrpx_router.h +++ b/src/shrpx_router.h @@ -58,7 +58,7 @@ public: // Adds route |pattern| with its |index|. bool add_route(const StringRef &pattern, size_t index); // Returns the matched index of pattern. -1 if there is no match. - ssize_t match(const std::string &host, const StringRef &path) const; + ssize_t match(const StringRef &host, const StringRef &path) const; void add_node(RNode *node, const char *pattern, size_t patlen, size_t index);