From: Amos Jeffries Date: Sun, 19 Jul 2015 13:23:01 +0000 (-0700) Subject: Cleanup: replace urlCanonical() with HttpRequest::effectiveReuqestUri() X-Git-Tag: merge-candidate-3-v1~38^2~7 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=851feda6490fd423979e170ea0e65af79a4bdd4a;p=thirdparty%2Fsquid.git Cleanup: replace urlCanonical() with HttpRequest::effectiveReuqestUri() We have previously been using the term "canonical URL" in Squid to mean absolute-URI, but not in all cases and may sometimes mean authority-form. RFC 7230 introduces a new term "Effective Request URI" which directly matches our desired usage. * make urlCanonical() global function a method of class HttpRequest since it depends on request method for its particular form syntax * remove the now unnecessary canonical member and HttpRequest::SetHost() * convert HttpRequest::storeId(), Ftp::UrlWith2f(), and ps_state::url() to SBuf usage to avoid performance regressions in their use. * replace many uses of xstrdup() with xstrndup() for performance where the copy cannot be avoided entirely. * avoid using urlParse() to do a simple URL data-copy in ICAP handling * update stub_HttpRequest.cc to match full class HttpRequest API --- diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 3a2b0f3d38..bc60ec3600 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -72,7 +72,6 @@ HttpRequest::init() #if USE_AUTH auth_user_request = NULL; #endif - canonical = NULL; memset(&flags, '\0', sizeof(flags)); range = NULL; ims = -1; @@ -120,8 +119,6 @@ HttpRequest::clean() #if USE_AUTH auth_user_request = NULL; #endif - safe_free(canonical); - safe_free(vary_headers); url.clear(); @@ -187,9 +184,6 @@ HttpRequest::clone() const copy->url.port(url.port()); copy->url.path(url.path()); - // urlPath handled in ctor - copy->canonical = canonical ? xstrdup(canonical) : NULL; - // range handled in hdrCacheInit() copy->ims = ims; copy->imslen = imslen; @@ -495,11 +489,7 @@ HttpRequest::clearError() void HttpRequest::packFirstLineInto(Packable * p, bool full_uri) const { - SBuf tmp; - if (full_uri) - tmp = urlCanonical((HttpRequest*)this); - else - tmp = url.path(); + const SBuf tmp(full_uri ? effectiveRequestUri() : url.path()); // form HTTP request-line p->appendf(SQUIDSBUFPH " " SQUIDSBUFPH " HTTP/%d.%d\r\n", @@ -678,16 +668,22 @@ HttpRequest::pinnedConnection() return NULL; } -const char * +const SBuf HttpRequest::storeId() { if (store_id.size() != 0) { - debugs(73, 3, "sent back store_id:" << store_id); - - return store_id.termedBuf(); + debugs(73, 3, "sent back store_id: " << store_id); + return SBuf(store_id); } - debugs(73, 3, "sent back canonicalUrl:" << urlCanonical(this) ); + debugs(73, 3, "sent back effectiveRequestUrl: " << effectiveRequestUri()); + return effectiveRequestUri(); +} - return urlCanonical(this); +const SBuf & +HttpRequest::effectiveRequestUri() const +{ + if (method.id() == Http::METHOD_CONNECT) + return url.authority(true); // host:port + return url.absolute(); } diff --git a/src/HttpRequest.h b/src/HttpRequest.h index a0f11d6855..2f26cb100d 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -66,14 +66,6 @@ public: /// whether the client is likely to be able to handle a 1xx reply bool canHandle1xx() const; - /* HACK: This method is only inline to get around Makefile dependancies */ - /* caused by HttpRequest being used in places it really shouldn't. */ - /* ideally URL would be used directly instead. */ - inline void SetHost(const char *src) { - url.host(src); - safe_free(canonical); // force its re-build - }; - #if USE_ADAPTATION /// Returns possibly nil history, creating it if adapt. logging is enabled Adaptation::History::Pointer adaptLogHistory() const; @@ -116,7 +108,8 @@ public: Auth::UserRequest::Pointer auth_user_request; #endif - char *canonical; + /// RFC 7230 section 5.5 - Effective Request URI + const SBuf &effectiveRequestUri() const; /** * If defined, store_id_program mapped the request URL to this ID. @@ -211,10 +204,9 @@ public: /** * Returns the current StoreID for the request as a nul-terminated char*. * Always returns the current id for the request - * (either the request canonical url or modified ID by the helper). - * Does not return NULL. + * (either the effective request URI or modified ID by the helper). */ - const char *storeId(); + const SBuf storeId(); /** * The client connection manager, if known; diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc index 49cf370aa7..ffd4c39509 100644 --- a/src/PeerPoolMgr.cc +++ b/src/PeerPoolMgr.cc @@ -67,7 +67,7 @@ PeerPoolMgr::start() // ErrorState, getOutgoingAddress(), and other APIs may require a request. // We fake one. TODO: Optionally send this request to peers? request = new HttpRequest(Http::METHOD_OPTIONS, AnyP::PROTO_HTTP, "*"); - request->SetHost(peer->host); + request->url.host(peer->host); checkpoint("peer initialized"); } diff --git a/src/PeerSelectState.h b/src/PeerSelectState.h index ad34fbec7a..7b94706e27 100644 --- a/src/PeerSelectState.h +++ b/src/PeerSelectState.h @@ -53,7 +53,7 @@ public: // Produce a URL for display identifying the transaction we are // trying to locate a peer for. - const char * url() const; + const SBuf url() const; HttpRequest *request; AccessLogEntry::Pointer al; ///< info for the future access.log entry diff --git a/src/URL.h b/src/URL.h index 3ae18ead79..f13df3de3e 100644 --- a/src/URL.h +++ b/src/URL.h @@ -143,7 +143,6 @@ class HttpRequestMethod; AnyP::ProtocolType urlParseProtocol(const char *, const char *e = NULL); void urlInitialize(void); HttpRequest *urlParse(const HttpRequestMethod&, char *, HttpRequest *request = NULL); -const char *urlCanonical(HttpRequest *); char *urlCanonicalClean(const HttpRequest *); const char *urlCanonicalFakeHttps(const HttpRequest * request); bool urlIsRelative(const char *); diff --git a/src/acl/Url.cc b/src/acl/Url.cc index e145e44ddf..5a83c8cfb3 100644 --- a/src/acl/Url.cc +++ b/src/acl/Url.cc @@ -12,16 +12,18 @@ #include "acl/Checklist.h" #include "acl/RegexData.h" #include "acl/Url.h" +#include "HttpRequest.h" #include "rfc1738.h" #include "src/URL.h" int ACLUrlStrategy::match (ACLData * &data, ACLFilledChecklist *checklist, ACLFlags &) { - char *esc_buf = xstrdup(urlCanonical(checklist->request)); + const SBuf &tmp = checklist->request->effectiveRequestUri(); + char *esc_buf = xstrndup(tmp.rawContent(), tmp.length()+1); rfc1738_unescape(esc_buf); int result = data->match(esc_buf); - safe_free(esc_buf); + xfree(esc_buf); return result; } diff --git a/src/adaptation/ecap/MessageRep.cc b/src/adaptation/ecap/MessageRep.cc index 0676ea5452..ade77fac9b 100644 --- a/src/adaptation/ecap/MessageRep.cc +++ b/src/adaptation/ecap/MessageRep.cc @@ -212,10 +212,11 @@ Adaptation::Ecap::RequestLineRep::uri(const Area &aUri) Adaptation::Ecap::RequestLineRep::Area Adaptation::Ecap::RequestLineRep::uri() const { - const char *fullUrl = urlCanonical(&theMessage); - Must(fullUrl); + const SBuf &fullUrl = theMessage.effectiveRequestUri(); + // XXX: effectiveRequestUri() cannot return NULL or even empty string, some other problem? + Must(!fullUrl.isEmpty()); // optimize: avoid copying by having an Area::Detail that locks theMessage - return Area::FromTempBuffer(fullUrl, strlen(fullUrl)); + return Area::FromTempBuffer(fullUrl.rawContent(), fullUrl.length()); } void diff --git a/src/adaptation/icap/ModXact.cc b/src/adaptation/icap/ModXact.cc index 310227048b..8fb8f11099 100644 --- a/src/adaptation/icap/ModXact.cc +++ b/src/adaptation/icap/ModXact.cc @@ -968,10 +968,6 @@ void Adaptation::Icap::ModXact::prepEchoing() httpBuf.terminate(); // HttpMsg::parse requires nil-terminated buffer Must(adapted.header->parse(httpBuf.content(), httpBuf.contentSize(), true, &error)); - - if (HttpRequest *r = dynamic_cast(adapted.header)) - urlCanonical(r); // parse does not set HttpRequest::canonical - Must(adapted.header->hdr_sz == httpBuf.contentSize()); // no leftovers httpBuf.clean(); @@ -1090,9 +1086,6 @@ bool Adaptation::Icap::ModXact::parseHead(HttpMsg *head) return false; } - if (HttpRequest *r = dynamic_cast(head)) - urlCanonical(r); // parse does not set HttpRequest::canonical - debugs(93, 5, HERE << "parse success, consume " << head->hdr_sz << " bytes, return true"); readBuf.consume(head->hdr_sz); return true; @@ -1539,8 +1532,9 @@ void Adaptation::Icap::ModXact::encapsulateHead(MemBuf &icapBuf, const char *sec if (const HttpRequest* old_request = dynamic_cast(head)) { HttpRequest::Pointer new_request(new HttpRequest); - Must(old_request->canonical); - urlParse(old_request->method, old_request->canonical, new_request.getRaw()); + // copy the requst-line details + new_request->method = old_request->method; + new_request->url = old_request->url; new_request->http_ver = old_request->http_ver; headClone = new_request.getRaw(); } else if (const HttpReply *old_reply = dynamic_cast(head)) { diff --git a/src/carp.cc b/src/carp.cc index dcc75c3e76..957ddc47ed 100644 --- a/src/carp.cc +++ b/src/carp.cc @@ -157,15 +157,15 @@ carpSelectParent(HttpRequest * request) return NULL; /* calculate hash key */ - debugs(39, 2, "carpSelectParent: Calculating hash for " << urlCanonical(request)); + debugs(39, 2, "carpSelectParent: Calculating hash for " << request->effectiveRequestUri()); /* select CachePeer */ for (k = 0; k < n_carp_peers; ++k) { SBuf key; tp = carp_peers[k]; if (tp->options.carp_key.set) { - //this code follows urlCanonical's pattern. - // corner cases should use the canonical URL + // this code follows URI syntax pattern. + // corner cases should use the full effective request URI if (tp->options.carp_key.scheme) { key.append(request->url.getScheme().c_str()); if (key.length()) //if the scheme is not empty @@ -190,10 +190,10 @@ carpSelectParent(HttpRequest * request) } // if the url-based key is empty, e.g. because the user is // asking to balance on the path but the request doesn't supply any, - // then fall back to canonical URL + // then fall back to the effective request URI if (key.isEmpty()) - key=SBuf(urlCanonical(request)); + key=request->effectiveRequestUri(); for (const char *c = key.rawContent(), *e=key.rawContent()+key.length(); c < e; ++c) user_hash += ROTATE_LEFT(user_hash, 19) + *c; diff --git a/src/client_side.cc b/src/client_side.cc index 338be92d59..22abf07d55 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2564,7 +2564,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, } else if (Config.onoff.global_internal_static && internalStaticCheck(request->url.path())) { debugs(33, 2, "internal URL found: " << request->url.getScheme() << "://" << request->url.authority(true) << " (global_internal_static on)"); request->url.setScheme(AnyP::PROTO_HTTP); - request->SetHost(internalHostname()); + request->url.host(internalHostname()); request->url.port(getMyPort()); http->flags.internal = true; } else @@ -3775,7 +3775,7 @@ ConnStateData::postHttpsAccept() HttpRequest *request = new HttpRequest(); static char ip[MAX_IPSTRLEN]; assert(clientConnection->flags & (COMM_TRANSPARENT | COMM_INTERCEPTION)); - request->SetHost(clientConnection->local.toStr(ip, sizeof(ip))); + request->url.host(clientConnection->local.toStr(ip, sizeof(ip))); request->url.port(clientConnection->local.port()); request->myportname = port->name; diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index fcd73c85d9..73ade1dc70 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -512,7 +512,7 @@ clientReplyContext::cacheHit(StoreIOBuffer result) */ assert(http->logType.oldType == LOG_TCP_HIT); - if (strcmp(e->mem_obj->storeId(), http->request->storeId()) != 0) { + if (http->request->storeId().cmp(e->mem_obj->storeId()) != 0) { debugs(33, DBG_IMPORTANT, "clientProcessHit: URL mismatch, '" << e->mem_obj->storeId() << "' != '" << http->request->storeId() << "'"); http->logType = LOG_TCP_MISS; // we lack a more precise LOG_*_MISS code processMiss(); @@ -867,8 +867,9 @@ purgeEntriesByUrl(HttpRequest * req, const char *url) void clientReplyContext::purgeAllCached() { - const char *url = urlCanonical(http->request); - purgeEntriesByUrl(http->request, url); + // XXX: performance regression, c_str() reallocates + SBuf url(http->request->effectiveRequestUri()); + purgeEntriesByUrl(http->request, url.c_str()); } void @@ -997,7 +998,7 @@ void clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) { if (newEntry && !newEntry->isNull()) { - debugs(88, 4, "clientPurgeRequest: HEAD '" << newEntry->url() << "'" ); + debugs(88, 4, "HEAD " << newEntry->url()); #if USE_HTCP neighborsHtcpClear(newEntry, NULL, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE); #endif @@ -1009,10 +1010,12 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) if (http->request->vary_headers && !strstr(http->request->vary_headers, "=")) { - StoreEntry *entry = storeGetPublic(urlCanonical(http->request), Http::METHOD_GET); + // XXX: performance regression, c_str() reallocates + SBuf tmp(http->request->effectiveRequestUri()); + StoreEntry *entry = storeGetPublic(tmp.c_str(), Http::METHOD_GET); if (entry) { - debugs(88, 4, "clientPurgeRequest: Vary GET '" << entry->url() << "'" ); + debugs(88, 4, "Vary GET " << entry->url()); #if USE_HTCP neighborsHtcpClear(entry, NULL, http->request, HttpRequestMethod(Http::METHOD_GET), HTCP_CLR_PURGE); #endif @@ -1020,10 +1023,10 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) purgeStatus = Http::scOkay; } - entry = storeGetPublic(urlCanonical(http->request), Http::METHOD_HEAD); + entry = storeGetPublic(tmp.c_str(), Http::METHOD_HEAD); if (entry) { - debugs(88, 4, "clientPurgeRequest: Vary HEAD '" << entry->url() << "'" ); + debugs(88, 4, "Vary HEAD " << entry->url()); #if USE_HTCP neighborsHtcpClear(entry, NULL, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE); #endif diff --git a/src/client_side_request.cc b/src/client_side_request.cc index e726b4d0da..ecb223d15d 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -559,7 +559,7 @@ ClientRequestContext::hostHeaderVerifyFailed(const char *A, const char *B) // NP: we do not yet handle CONNECT tunnels well, so ignore for them if (!Config.onoff.hostStrictVerify && http->request->method != Http::METHOD_CONNECT) { debugs(85, 3, "SECURITY ALERT: Host header forgery detected on " << http->getConn()->clientConnection << - " (" << A << " does not match " << B << ") on URL: " << urlCanonical(http->request)); + " (" << A << " does not match " << B << ") on URL: " << http->request->effectiveRequestUri()); // NP: it is tempting to use 'flags.noCache' but that is all about READing cache data. // The problems here are about WRITE for new cache content, which means flags.cachable @@ -574,7 +574,7 @@ ClientRequestContext::hostHeaderVerifyFailed(const char *A, const char *B) debugs(85, DBG_IMPORTANT, "SECURITY ALERT: Host header forgery detected on " << http->getConn()->clientConnection << " (" << A << " does not match " << B << ")"); debugs(85, DBG_IMPORTANT, "SECURITY ALERT: By user agent: " << http->request->header.getStr(HDR_USER_AGENT)); - debugs(85, DBG_IMPORTANT, "SECURITY ALERT: on URL: " << urlCanonical(http->request)); + debugs(85, DBG_IMPORTANT, "SECURITY ALERT: on URL: " << http->request->effectiveRequestUri()); // IP address validation for Host: failed. reject the connection. clientStreamNode *node = (clientStreamNode *)http->client_stream.tail->prev->data; @@ -834,9 +834,8 @@ ClientRequestContext::clientAccessCheckDone(const allow_t &answer) /* ACCESS_ALLOWED continues here ... */ safe_free(http->uri); - - http->uri = xstrdup(urlCanonical(http->request)); - + const SBuf tmp(http->request->effectiveRequestUri()); + http->uri = xstrndup(tmp.rawContent(), tmp.length()+1); http->doCallouts(); } @@ -1300,7 +1299,7 @@ ClientRequestContext::clientRedirectDone(const Helper::Reply &reply) // XXX: the clone() should be done only AFTER we know the new URL is valid. HttpRequest *new_request = old_request->clone(); if (urlParse(old_request->method, const_cast(urlNote), new_request)) { - debugs(61,2, HERE << "URL-rewriter diverts URL from " << urlCanonical(old_request) << " to " << urlCanonical(new_request)); + debugs(61, 2, "URL-rewriter diverts URL from " << old_request->effectiveRequestUri() << " to " << new_request->effectiveRequestUri()); // update the new request to flag the re-writing was done on it new_request->flags.redirected = true; @@ -1314,7 +1313,8 @@ ClientRequestContext::clientRedirectDone(const Helper::Reply &reply) // update the current working ClientHttpRequest fields safe_free(http->uri); - http->uri = xstrdup(urlCanonical(new_request)); + const SBuf tmp(new_request->effectiveRequestUri()); + http->uri = xstrndup(tmp.rawContent(), tmp.length()+1); HTTPMSGUNLOCK(old_request); http->request = new_request; HTTPMSGLOCK(http->request); @@ -1806,8 +1806,9 @@ ClientHttpRequest::doCallouts() #endif if (calloutContext->error) { - const char *storeUri = request->storeId(); - StoreEntry *e= storeCreateEntry(storeUri, storeUri, request->flags, request->method); + // XXX: prformance regression. c_str() reallocates + SBuf storeUri(request->storeId()); + StoreEntry *e = storeCreateEntry(storeUri.c_str(), storeUri.c_str(), request->flags, request->method); #if USE_OPENSSL if (sslBumpNeeded()) { // We have to serve an error, so bump the client first. @@ -1911,14 +1912,15 @@ ClientHttpRequest::handleAdaptedHeader(HttpMsg *msg) HTTPMSGLOCK(request); // update the new message to flag whether URL re-writing was done on it - if (strcmp(urlCanonical(request),uri) != 0) + if (request->effectiveRequestUri().cmp(uri) != 0) request->flags.redirected = 1; /* * Store the new URI for logging */ xfree(uri); - uri = xstrdup(urlCanonical(request)); + const SBuf tmp(request->effectiveRequestUri()); + uri = xstrndup(tmp.rawContent(), tmp.length()); setLogUri(this, urlCanonicalClean(request)); assert(request->method.id()); } else if (HttpReply *new_rep = dynamic_cast(msg)) { diff --git a/src/clients/Client.cc b/src/clients/Client.cc index 6754a0695a..4343fa0431 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -493,8 +493,9 @@ Client::maybePurgeOthers() return; // XXX: should we use originalRequest() here? - const char *reqUrl = urlCanonical(request); - debugs(88, 5, "maybe purging due to " << request->method << ' ' << reqUrl); + SBuf tmp(request->effectiveRequestUri()); + const char *reqUrl = tmp.c_str(); + debugs(88, 5, "maybe purging due to " << request->method << ' ' << tmp); purgeEntriesByUrl(request, reqUrl); purgeEntriesByHeader(request, reqUrl, theFinalReply, HDR_LOCATION); purgeEntriesByHeader(request, reqUrl, theFinalReply, HDR_CONTENT_LOCATION); diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc index 8b6dbfbdcb..4addef7d2c 100644 --- a/src/clients/FtpGateway.cc +++ b/src/clients/FtpGateway.cc @@ -2642,25 +2642,25 @@ Ftp::Gateway::ftpAuthRequired(HttpRequest * request, const char *realm) return newrep; } -const char * +const SBuf & Ftp::UrlWith2f(HttpRequest * request) { SBuf newbuf("%2f"); - if (request->url.getScheme() != AnyP::PROTO_FTP) - return NULL; + if (request->url.getScheme() != AnyP::PROTO_FTP) { + static const SBuf nil; + return nil; + } if (request->url.path()[0] == '/') { newbuf.append(request->url.path()); request->url.path(newbuf); - safe_free(request->canonical); } else if (!request->url.path().startsWith(newbuf)) { newbuf.append(request->url.path().substr(1)); request->url.path(newbuf); - safe_free(request->canonical); } - return urlCanonical(request); + return request->effectiveRequestUri(); } void diff --git a/src/clients/forward.h b/src/clients/forward.h index 4575bf6685..8ad012eb3b 100644 --- a/src/clients/forward.h +++ b/src/clients/forward.h @@ -11,6 +11,7 @@ class FwdState; class HttpRequest; +class SBuf; class AsyncJob; template class CbcPointer; @@ -35,7 +36,7 @@ AsyncJobPointer StartRelay(FwdState *const fwdState); * * \todo Should be a URL class API call. */ -const char *UrlWith2f(HttpRequest *); +const SBuf &UrlWith2f(HttpRequest *); } // namespace Ftp diff --git a/src/errorpage.cc b/src/errorpage.cc index 18b9c30778..abc89ea9f4 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -792,7 +792,11 @@ ErrorState::Convert(char token, bool building_deny_info_url, bool allowRecursion case 'B': if (building_deny_info_url) break; - p = request ? Ftp::UrlWith2f(request) : "[no URL]"; + if (request) { + const SBuf &tmp = Ftp::UrlWith2f(request); + mb.append(tmp.rawContent(), tmp.length()); + } else + p = "[no URL]"; break; case 'c': @@ -969,7 +973,11 @@ ErrorState::Convert(char token, bool building_deny_info_url, bool allowRecursion case 's': /* for backward compat we make %s show the full URL. Drop this in some future release. */ if (building_deny_info_url) { - p = request ? urlCanonical(request) : url; + if (request) { + const SBuf &tmp = request->effectiveRequestUri(); + mb.append(tmp.rawContent(), tmp.length()); + } else + p = url; debugs(0, DBG_CRITICAL, "WARNING: deny_info now accepts coded tags. Use %u to get the full URL instead of %s"); } else p = visible_appname_string; @@ -1005,7 +1013,7 @@ ErrorState::Convert(char token, bool building_deny_info_url, bool allowRecursion break; case 'U': - /* Using the fake-https version of canonical so error pages see https:// */ + /* Using the fake-https version of absolute-URI so error pages see https:// */ /* even when the url-path cannot be shown as more than '*' */ if (request) p = urlCanonicalFakeHttps(request); @@ -1016,9 +1024,10 @@ ErrorState::Convert(char token, bool building_deny_info_url, bool allowRecursion break; case 'u': - if (request) - p = urlCanonical(request); - else if (url) + if (request) { + const SBuf &tmp = request->effectiveRequestUri(); + mb.append(tmp.rawContent(), tmp.length()); + } else if (url) p = url; else if (!building_deny_info_url) p = "[no URL]"; diff --git a/src/external_acl.cc b/src/external_acl.cc index a2a2656ced..fe480cf88c 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -962,7 +962,8 @@ makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data) break; case Format::LFT_CLIENT_REQ_URI: - str = urlCanonical(request); + snprintf(buf, sizeof(buf), SQUIDSBUFPH, SQUIDSBUFPRINT(request->effectiveRequestUri())); + str = buf; break; case Format::LFT_CLIENT_REQ_URLDOMAIN: diff --git a/src/format/Format.cc b/src/format/Format.cc index 55ba00d047..b531835656 100644 --- a/src/format/Format.cc +++ b/src/format/Format.cc @@ -935,7 +935,9 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS case LFT_CLIENT_REQ_URI: // original client URI if (al->request) { - out = urlCanonical(al->request); + const SBuf &s = al->request->effectiveRequestUri(); + sb.append(s.rawContent(), s.length()); + out = sb.termedBuf(); quote = 1; } break; @@ -1010,7 +1012,9 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS case LFT_SERVER_REQ_URI: // adapted request URI sent to server/peer if (al->adapted_request) { - out = urlCanonical(al->adapted_request); + const SBuf &s = al->adapted_request->effectiveRequestUri(); + sb.append(s.rawContent(), s.length()); + out = sb.termedBuf(); quote = 1; } break; diff --git a/src/http.cc b/src/http.cc index 87bacf1085..95ef0d036a 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1831,7 +1831,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request, static int warnedCount = 0; if (warnedCount++ < 100) { - const char *url = entry ? entry->url() : urlCanonical(request); + const SBuf url(entry ? SBuf(entry->url()) : request->effectiveRequestUri()); debugs(11, DBG_IMPORTANT, "Warning: likely forwarding loop with " << url); } } @@ -1901,10 +1901,9 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request, /* Add max-age only without no-cache */ if (!cc->hasMaxAge() && !cc->hasNoCache()) { - const char *url = - entry ? entry->url() : urlCanonical(request); - cc->maxAge(getMaxAge(url)); - + // XXX: performance regression. c_str() reallocates + SBuf tmp(request->effectiveRequestUri()); + cc->maxAge(getMaxAge(entry ? entry->url() : tmp.c_str())); } /* Enforce sibling relations */ @@ -2163,8 +2162,7 @@ HttpStateData::buildRequestPrefix(MemBuf * mb) * not the one we are sending. Needs checking. */ const AnyP::ProtocolVersion httpver = Http::ProtocolVersion(); - const bool canonical = (_peer && !_peer->options.originserver); - const SBuf url = canonical ? SBuf(urlCanonical(request)) : request->url.path(); + const SBuf url(_peer && !_peer->options.originserver ? request->effectiveRequestUri() : request->url.path()); mb->appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\r\n", SQUIDSBUFPRINT(request->method.image()), SQUIDSBUFPRINT(url), diff --git a/src/icmp/net_db.cc b/src/icmp/net_db.cc index f7fc4a1bee..298b2109ef 100644 --- a/src/icmp/net_db.cc +++ b/src/icmp/net_db.cc @@ -1318,8 +1318,6 @@ netdbExchangeStart(void *data) if (p->login) ex->r->url.userInfo(SBuf(p->login)); - urlCanonical(ex->r); - FwdState::fwdStart(Comm::ConnectionPointer(), ex->e, ex->r); #endif diff --git a/src/peer_select.cc b/src/peer_select.cc index c325cafd93..211aff9573 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -961,16 +961,17 @@ ps_state::ps_state() : request (NULL), ; // no local defaults. } -const char * +const SBuf ps_state::url() const { if (entry) - return entry->url(); + return SBuf(entry->url()); if (request) - return urlCanonical(request); + return request->effectiveRequestUri(); - return "[no URL]"; + static const SBuf noUrl("[no URL]"); + return noUrl; } ping_data::ping_data() : diff --git a/src/refresh.cc b/src/refresh.cc index 6e222f8957..2dcbff8b58 100644 --- a/src/refresh.cc +++ b/src/refresh.cc @@ -285,19 +285,20 @@ refreshStaleness(const StoreEntry * entry, time_t check_time, const time_t age, static int refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta) { - const char *uri = NULL; time_t age = 0; time_t check_time = squid_curtime + delta; int staleness; stale_flags sf; // get the URL of this entry, if there is one + static const SBuf nilUri(""); + SBuf uri = nilUri; if (entry->mem_obj) uri = entry->mem_obj->storeId(); else if (request) - uri = urlCanonical(request); + uri = request->effectiveRequestUri(); - debugs(22, 3, "checking freshness of '" << (uri ? uri : "") << "'"); + debugs(22, 3, "checking freshness of URI: " << uri); // age is not necessarily the age now, but the age at the given check_time if (check_time > entry->timestamp) @@ -312,7 +313,8 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta) * 2. the "." rule from the config file * 3. the default "." rule */ - const RefreshPattern *R = uri ? refreshLimits(uri) : refreshUncompiledPattern("."); + // XXX: performance regression. c_str() reallocates + const RefreshPattern *R = (uri != nilUri) ? refreshLimits(uri.c_str()) : refreshUncompiledPattern("."); if (NULL == R) R = &DefaultRefresh; diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc index faac1a2c96..c4d53fabbe 100644 --- a/src/ssl/PeerConnector.cc +++ b/src/ssl/PeerConnector.cc @@ -731,7 +731,7 @@ Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error) if (request->flags.sslPeek && !isConnectRequest) { if (X509 *srvX509 = serverBump->serverCert.get()) { if (const char *name = Ssl::CommonHostName(srvX509)) { - request->SetHost(name); + request->url.host(name); debugs(83, 3, "reset request host: " << name); } } diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index 1350b25c2e..12aa644b2c 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -28,12 +28,14 @@ Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e, Ssl::BumpMo act.step1 = md; act.step2 = act.step3 = Ssl::bumpNone; - const char *uri = urlCanonical(request.getRaw()); if (e) { entry = e; entry->lock("Ssl::ServerBump"); - } else - entry = storeCreateEntry(uri, uri, request->flags, request->method); + } else { + // XXX: Performance regression. c_str() reallocates + SBuf uri(request->effectiveRequestUri()); + entry = storeCreateEntry(uri.c_str(), uri.c_str(), request->flags, request->method); + } // We do not need to be a client because the error contents will be used // later, but an entry without any client will trim all its contents away. sc = storeClientListAdd(entry, this); diff --git a/src/store_key_md5.cc b/src/store_key_md5.cc index 4d306e4897..f4d97955bc 100644 --- a/src/store_key_md5.cc +++ b/src/store_key_md5.cc @@ -118,11 +118,11 @@ storeKeyPublicByRequestMethod(HttpRequest * request, const HttpRequestMethod& me { static cache_key digest[SQUID_MD5_DIGEST_LENGTH]; unsigned char m = (unsigned char) method.id(); - const char *url = request->storeId(); /* storeId returns the right storeID\canonical URL for the md5 calc */ + const SBuf url = request->storeId(); /* returns the right storeID\URL for the MD5 calc */ SquidMD5_CTX M; SquidMD5Init(&M); SquidMD5Update(&M, &m, sizeof(m)); - SquidMD5Update(&M, (unsigned char *) url, strlen(url)); + SquidMD5Update(&M, (unsigned char *) url.rawContent(), url.length()); if (request->vary_headers) { SquidMD5Update(&M, (unsigned char *) request->vary_headers, strlen(request->vary_headers)); diff --git a/src/store_swapmeta.cc b/src/store_swapmeta.cc index 095c6cde8d..4360c02bc0 100644 --- a/src/store_swapmeta.cc +++ b/src/store_swapmeta.cc @@ -39,13 +39,13 @@ storeSwapMetaBuild(StoreEntry * e) { tlv *TLV = NULL; /* we'll return this */ tlv **T = &TLV; - const char *url; const char *vary; assert(e->mem_obj != NULL); const int64_t objsize = e->mem_obj->expectedReplySize(); assert(e->swap_status == SWAPOUT_WRITING); // e->mem_obj->request may be nil in this context + SBuf url; if (e->mem_obj->request) url = e->mem_obj->request->storeId(); else @@ -68,8 +68,9 @@ storeSwapMetaBuild(StoreEntry * e) return NULL; } + // XXX: do TLV without the c_str() termination. check readers first though T = StoreMeta::Add(T, t); - t = StoreMeta::Factory(STORE_META_URL, strlen(url) + 1, url); + t = StoreMeta::Factory(STORE_META_URL, url.length()+1, url.c_str()); if (!t) { storeSwapTLVFree(TLV); diff --git a/src/tests/stub_HttpRequest.cc b/src/tests/stub_HttpRequest.cc index 765769aadd..55f19695fb 100644 --- a/src/tests/stub_HttpRequest.cc +++ b/src/tests/stub_HttpRequest.cc @@ -13,18 +13,48 @@ #define STUB_API "HttpRequest.cc" #include "tests/STUB.h" -HttpRequest::HttpRequest() : HttpMsg(hoRequest) STUB - HttpRequest::HttpRequest(const HttpRequestMethod& aMethod, AnyP::ProtocolType aProtocol, const char *aUrlpath) : HttpMsg(hoRequest) STUB +// void httpRequestPack(void *obj, Packable *p); + +HttpRequest::HttpRequest() : HttpMsg(hoRequest) {STUB} + HttpRequest::HttpRequest(const HttpRequestMethod &, AnyP::ProtocolType, const char *) : HttpMsg(hoRequest) {STUB} HttpRequest::~HttpRequest() STUB - void HttpRequest::packFirstLineInto(Packable *, bool) const STUB - bool HttpRequest::sanityCheckStartLine(const char*buf, const size_t hdr_len, Http::StatusCode *error) STUB_RETVAL(false) - void HttpRequest::hdrCacheInit() STUB void HttpRequest::reset() STUB - bool HttpRequest::expectingBody(const HttpRequestMethod& unused, int64_t&) const STUB_RETVAL(false) - void HttpRequest::initHTTP(const HttpRequestMethod& aMethod, AnyP::ProtocolType aProtocol, const char *aUrlpath) STUB - bool HttpRequest::parseFirstLine(const char *start, const char *end) STUB_RETVAL(false) + void HttpRequest::initHTTP(const HttpRequestMethod &, AnyP::ProtocolType, const char *) STUB HttpRequest * HttpRequest::clone() const STUB_RETVAL(NULL) - bool HttpRequest::inheritProperties(const HttpMsg *aMsg) STUB_RETVAL(false) + bool HttpRequest::maybeCacheable() STUB_RETVAL(false) + bool HttpRequest::conditional() const STUB_RETVAL(false) + bool HttpRequest::canHandle1xx() const STUB_RETVAL(false) +#if USE_ADAPTATION + Adaptation::History::Pointer HttpRequest::adaptLogHistory() const STUB_RETVAL(Adaptation::History::Pointer()) + Adaptation::History::Pointer HttpRequest::adaptHistory(bool) const STUB_RETVAL(Adaptation::History::Pointer()) + void HttpRequest::adaptHistoryImport(const HttpRequest &) STUB +#endif +#if ICAP_CLIENT + Adaptation::Icap::History::Pointer HttpRequest::icapHistory() const STUB_RETVAL(Adaptation::Icap::History::Pointer()) +#endif + void HttpRequest::recordLookup(const Dns::LookupDetails &) STUB + void HttpRequest::detailError(err_type, int) STUB + void HttpRequest::clearError() STUB + void HttpRequest::clean() STUB + void HttpRequest::init() STUB + static const SBuf nilSBuf; + const SBuf &HttpRequest::effectiveRequestUri() const STUB_RETVAL(nilSBuf) + bool HttpRequest::multipartRangeRequest() const STUB_RETVAL(false) + bool HttpRequest::parseFirstLine(const char *, const char *) STUB_RETVAL(false) + bool HttpRequest::parseHeader(Http1::RequestParser &) STUB_RETVAL(false) + bool HttpRequest::expectingBody(const HttpRequestMethod &, int64_t &) const STUB_RETVAL(false) + bool HttpRequest::bodyNibbled() const STUB_RETVAL(false) + int HttpRequest::prefixLen() const STUB_RETVAL(0) + void HttpRequest::swapOut(StoreEntry *) STUB + void HttpRequest::pack(Packable *) STUB + void HttpRequest::httpRequestPack(void *, Packable *) STUB + HttpRequest * HttpRequest::CreateFromUrlAndMethod(char *, const HttpRequestMethod &) STUB_RETVAL(NULL) + HttpRequest * HttpRequest::CreateFromUrl(char *) STUB_RETVAL(NULL) + ConnStateData *HttpRequest::pinnedConnection() STUB_RETVAL(NULL) + const SBuf HttpRequest::storeId() STUB_RETVAL(SBuf(".")) + void HttpRequest::ignoreRange(const char *) STUB int64_t HttpRequest::getRangeOffsetLimit() STUB_RETVAL(0) - const char *HttpRequest::storeId() STUB_RETVAL(".") - + void HttpRequest::packFirstLineInto(Packable *, bool) const STUB + bool HttpRequest::sanityCheckStartLine(const char *, const size_t, Http::StatusCode *) STUB_RETVAL(false) + void HttpRequest::hdrCacheInit() STUB + bool HttpRequest::inheritProperties(const HttpMsg *) STUB_RETVAL(false) diff --git a/src/tunnel.cc b/src/tunnel.cc index 9746b9f03b..3aef143730 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -1190,14 +1190,14 @@ switchToTunnel(HttpRequest *request, Comm::ConnectionPointer &clientConn, Comm:: debugs(26,5, "Revert to tunnel FD " << clientConn->fd << " with FD " << srvConn->fd); /* Create state structure. */ TunnelStateData *tunnelState = NULL; - const char *url = urlCanonical(request); + const SBuf url(request->effectiveRequestUri()); debugs(26, 3, request->method << " " << url << " " << request->http_ver); ++statCounter.server.all.requests; ++statCounter.server.other.requests; tunnelState = new TunnelStateData; - tunnelState->url = xstrdup(url); + tunnelState->url = xstrndup(url.rawContent(), url.length()+1); tunnelState->request = request; tunnelState->server.size_ptr = NULL; //Set later if ClientSocketContext is available diff --git a/src/url.cc b/src/url.cc index 5239991526..3812e0c9b7 100644 --- a/src/url.cc +++ b/src/url.cc @@ -449,10 +449,9 @@ urlParseFinish(const HttpRequestMethod& method, request = new HttpRequest(method, protocol, urlpath); else { request->initHTTP(method, protocol, urlpath); - safe_free(request->canonical); } - request->SetHost(host); + request->url.host(host); request->url.userInfo(login); request->url.port(port); return request; @@ -464,7 +463,6 @@ urnParse(const HttpRequestMethod& method, char *urn, HttpRequest *request) debugs(50, 5, "urnParse: " << urn); if (request) { request->initHTTP(method, AnyP::PROTO_URN, urn + 4); - safe_free(request->canonical); return request; } @@ -522,22 +520,7 @@ URL::absolute() const return absolute_; } -const char * -urlCanonical(HttpRequest * request) -{ - if (request->canonical) - return request->canonical; - - SBuf url; - if (request->method.id() == Http::METHOD_CONNECT) - url = request->url.authority(true); // host:port - else - url = request->url.absolute(); - - return (request->canonical = xstrndup(url.rawContent(), url.length()+1)); -} - -/** \todo AYJ: Performance: This is an *almost* duplicate of urlCanonical. But elides the query-string. +/** \todo AYJ: Performance: This is an *almost* duplicate of HttpRequest::effectiveRequestUri(). But elides the query-string. * After copying it on in the first place! Would be less code to merge the two with a flag parameter. * and never copy the query-string part in the first place */ @@ -546,15 +529,11 @@ urlCanonicalClean(const HttpRequest * request) { LOCAL_ARRAY(char, buf, MAX_URL); - snprintf(buf, sizeof(buf), "%s", urlCanonical(const_cast(request))); + snprintf(buf, sizeof(buf), SQUIDSBUFPH, SQUIDSBUFPRINT(request->effectiveRequestUri())); buf[sizeof(buf)-1] = '\0'; // URN, CONNECT method, and non-stripped URIs can go straight out - if (!(request->url.getScheme() == AnyP::PROTO_URN || - !Config.onoff.strip_query_terms || - request->method == Http::METHOD_CONNECT - )) { - + if (Config.onoff.strip_query_terms && !(request->method == Http::METHOD_CONNECT || request->url.getScheme() == AnyP::PROTO_URN)) { // strip anything AFTER a question-mark // leaving the '?' in place if (auto t = strchr(buf, '?')) {