Cleanup: remove urlHostname hacks (#615)

The urlHostname() function and class it uses employ a very dirty
hack to parse a guess at the hostname portion of a URL.

AnyP::Uri objects can be used instead for better parsing of URLs
with far more accurate hostname extraction in a wider set of URI.

diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc
index b745c54d0a43ef13e19f400757dd40b8541f7732..cbe55b9cb3d3e04d4747b7fd90359ca62a2b99b4 100644 (file)
--- a/src/anyp/Uri.cc
+++ b/src/anyp/Uri.cc
@@ -890,105 +890,6 @@ urlCheckRequest(const HttpRequest * r)
     return rc;
 }
 
-/*
- * Quick-n-dirty host extraction from a URL.  Steps:
- *      Look for a colon
- *      Skip any '/' after the colon
- *      Copy the next SQUID_MAXHOSTNAMELEN bytes to host[]
- *      Look for an ending '/' or ':' and terminate
- *      Look for login info preceeded by '@'
- */
-
-class URLHostName
-{
-
-public:
-    char * extract(char const *url);
-
-private:
-    static char Host [SQUIDHOSTNAMELEN];
-    void init(char const *);
-    void findHostStart();
-    void trimTrailingChars();
-    void trimAuth();
-    char const *hostStart;
-    char const *url;
-};
-
-char *
-urlHostname(const char *url)
-{
-    return URLHostName().extract(url);
-}
-
-char URLHostName::Host[SQUIDHOSTNAMELEN];
-
-void
-URLHostName::init(char const *aUrl)
-{
-    Host[0] = '\0';
-    url = aUrl;
-}
-
-void
-URLHostName::findHostStart()
-{
-    if (NULL == (hostStart = strchr(url, ':')))
-        return;
-
-    ++hostStart;
-
-    while (*hostStart != '\0' && *hostStart == '/')
-        ++hostStart;
-
-    if (*hostStart == ']')
-        ++hostStart;
-}
-
-void
-URLHostName::trimTrailingChars()
-{
-    char *t;
-
-    if ((t = strchr(Host, '/')))
-        *t = '\0';
-
-    if ((t = strrchr(Host, ':')))
-        *t = '\0';
-
-    if ((t = strchr(Host, ']')))
-        *t = '\0';
-}
-
-void
-URLHostName::trimAuth()
-{
-    char *t;
-
-    if ((t = strrchr(Host, '@'))) {
-        ++t;
-        memmove(Host, t, strlen(t) + 1);
-    }
-}
-
-char *
-URLHostName::extract(char const *aUrl)
-{
-    init(aUrl);
-    findHostStart();
-
-    if (hostStart == NULL)
-        return NULL;
-
-    xstrncpy(Host, hostStart, SQUIDHOSTNAMELEN);
-
-    trimTrailingChars();
-
-    trimAuth();
-
-    return Host;
-}
-
 AnyP::Uri::Uri(AnyP::UriScheme const &aScheme) :
     scheme_(aScheme),
     hostIsNumeric_(false),

diff --git a/src/anyp/Uri.h b/src/anyp/Uri.h
index 8d31d901841d32f63175784fb3d18d2b0a751485..05115901974a9eff4ae040561d7e9553273f55d4 100644 (file)
--- a/src/anyp/Uri.h
+++ b/src/anyp/Uri.h
@@ -244,7 +244,6 @@ enum MatchDomainNameFlags {
  */
 int matchDomainName(const char *host, const char *domain, uint8_t flags = mdnNone);
 int urlCheckRequest(const HttpRequest *);
-char *urlHostname(const char *url);
 void urlExtMethodConfigure(void);
 
 #endif /* SQUID_SRC_ANYP_URI_H */

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 9d7db62cf3c93de524a29f3ac4c7cc52ae823639..b397d59d9fc7d0790ec7a329d07afa034b060e19 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -907,7 +907,7 @@ purgeEntriesByUrl(HttpRequest * req, const char *url)
             const cache_key *key = storeKeyPublic(url, m);
             debugs(88, 5, m << ' ' << url << ' ' << storeKeyText(key));
 #if USE_HTCP
-            neighborsHtcpClear(nullptr, url, req, m, HTCP_CLR_INVALIDATION);
+            neighborsHtcpClear(nullptr, req, m, HTCP_CLR_INVALIDATION);
 #endif
             Store::Root().evictIfFound(key);
         }
@@ -1040,7 +1040,7 @@ clientReplyContext::purgeDoPurgeGet(StoreEntry *newEntry)
         /* Release the cached URI */
         debugs(88, 4, "clientPurgeRequest: GET '" << newEntry->url() << "'" );
 #if USE_HTCP
-        neighborsHtcpClear(newEntry, NULL, http->request, HttpRequestMethod(Http::METHOD_GET), HTCP_CLR_PURGE);
+        neighborsHtcpClear(newEntry, http->request, HttpRequestMethod(Http::METHOD_GET), HTCP_CLR_PURGE);
 #endif
         newEntry->release(true);
         purgeStatus = Http::scOkay;
@@ -1056,7 +1056,7 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry)
     if (newEntry && !newEntry->isNull()) {
         debugs(88, 4, "HEAD " << newEntry->url());
 #if USE_HTCP
-        neighborsHtcpClear(newEntry, NULL, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE);
+        neighborsHtcpClear(newEntry, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE);
 #endif
         newEntry->release(true);
         purgeStatus = Http::scOkay;
@@ -1072,7 +1072,7 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry)
         if (entry) {
             debugs(88, 4, "Vary GET " << entry->url());
 #if USE_HTCP
-            neighborsHtcpClear(entry, NULL, http->request, HttpRequestMethod(Http::METHOD_GET), HTCP_CLR_PURGE);
+            neighborsHtcpClear(entry, http->request, HttpRequestMethod(Http::METHOD_GET), HTCP_CLR_PURGE);
 #endif
             entry->release(true);
             purgeStatus = Http::scOkay;
@@ -1083,7 +1083,7 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry)
         if (entry) {
             debugs(88, 4, "Vary HEAD " << entry->url());
 #if USE_HTCP
-            neighborsHtcpClear(entry, NULL, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE);
+            neighborsHtcpClear(entry, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE);
 #endif
             entry->release(true);
             purgeStatus = Http::scOkay;

diff --git a/src/clients/Client.cc b/src/clients/Client.cc
index cc0dab614cbd1cab74594225b3628e88646cc5e3..50ac9e4450567c450b703302ec950830b87a0aa9 100644 (file)
--- a/src/clients/Client.cc
+++ b/src/clients/Client.cc
@@ -439,7 +439,7 @@ Client::getMoreRequestBody(MemBuf &buf)
 static bool
 sameUrlHosts(const char *url1, const char *url2)
 {
-    // XXX: Want urlHostname() here, but it uses static storage and copying
+    // XXX: Want AnyP::Uri::parse() here, but it uses static storage and copying
     const char *host1 = strchr(url1, ':');
     const char *host2 = strchr(url2, ':');
 

diff --git a/src/htcp.cc b/src/htcp.cc
index 13380b345b08433d115b063e753921297342ead0..6f38077f8d8a8bb065fdf3d23fb7b071f7690d82 100644 (file)
--- a/src/htcp.cc
+++ b/src/htcp.cc
@@ -138,7 +138,7 @@ public:
 
 public:
     const char *method = nullptr;
-    char *uri = nullptr;
+    const char *uri = nullptr;
     char *version = nullptr;
     char *req_hdrs = nullptr;
     size_t reqHdrsSz = 0; ///< size of the req_hdrs content
@@ -791,6 +791,7 @@ htcpTstReply(htcpDataHeader * dhdr, StoreEntry * e, htcpSpecifier * spec, Ip::Ad
 
     if (spec) {
         stuff.S.method = spec->method;
+        stuff.S.request = spec->request;
         stuff.S.uri = spec->uri;
         stuff.S.version = spec->version;
         stuff.S.req_hdrs = spec->req_hdrs;
@@ -825,15 +826,15 @@ htcpTstReply(htcpDataHeader * dhdr, StoreEntry * e, htcpSpecifier * spec, Ip::Ad
         hdr.clean();
 
 #if USE_ICMP
-        if (char *host = urlHostname(spec->uri)) {
+        if (const char *host = spec->request->url.host()) {
             int rtt = 0;
             int hops = 0;
             int samp = 0;
             netdbHostData(host, &samp, &rtt, &hops);
 
             if (rtt || hops) {
-                char cto_buf[128];
-                snprintf(cto_buf, 128, "%s %d %f %d",
+                char cto_buf[SQUIDHOSTNAMELEN+128];
+                snprintf(cto_buf, sizeof(cto_buf), "%s %d %f %d",
                          host, samp, 0.001 * rtt, hops);
                 hdr.putExt("Cache-to-Origin", cto_buf);
             }
@@ -1470,7 +1471,7 @@ htcpQuery(StoreEntry * e, HttpRequest * req, CachePeer * p)
  * Send an HTCP CLR message for a specified item to a given CachePeer.
  */
 void
-htcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const HttpRequestMethod &, CachePeer * p, htcp_clr_reason reason)
+htcpClear(StoreEntry * e, HttpRequest * req, const HttpRequestMethod &, CachePeer * p, htcp_clr_reason reason)
 {
     static char pkt[8192];
     ssize_t pktlen;
@@ -1492,14 +1493,9 @@ htcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const HttpRequestM
 
     SBuf sb = req->method.image();
     stuff.S.method = sb.c_str();
-    if (e == NULL || e->mem_obj == NULL) {
-        if (uri == NULL) {
-            return;
-        }
-        stuff.S.uri = xstrdup(uri);
-    } else {
-        stuff.S.uri = (char *) e->url();
-    }
+    stuff.S.request = req;
+    SBuf uri = req->effectiveRequestUri();
+    stuff.S.uri = uri.c_str();
     stuff.S.version = vbuf;
     if (reason != HTCP_CLR_INVALIDATION) {
         HttpStateData::httpBuildRequestHeader(req, e, NULL, &hdr, flags);
@@ -1514,9 +1510,6 @@ htcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const HttpRequestM
     if (reason != HTCP_CLR_INVALIDATION) {
         mb.clean();
     }
-    if (e == NULL) {
-        xfree(stuff.S.uri);
-    }
     if (!pktlen) {
         debugs(31, 3, "htcpClear: htcpBuildPacket() failed");
         return;

diff --git a/src/htcp.h b/src/htcp.h
index 79dd649a8ebe0491b72b19400efcdda994103d8b..e185d938f37b7039f5ba35519fed849ffe354080 100644 (file)
--- a/src/htcp.h
+++ b/src/htcp.h
@@ -57,7 +57,7 @@ void htcpOpenPorts(void);
 int htcpQuery(StoreEntry * e, HttpRequest * req, CachePeer * p);
 
 /// \ingroup ServerProtocolHTCP
-void htcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const HttpRequestMethod &method, CachePeer * p, htcp_clr_reason reason);
+void htcpClear(StoreEntry * e, HttpRequest * req, const HttpRequestMethod &method, CachePeer * p, htcp_clr_reason reason);
 
 /// \ingroup ServerProtocolHTCP
 void htcpSocketShutdown(void);

diff --git a/src/http.cc b/src/http.cc
index 9654c4aba7cb9bf08751e2d1cf3a65665d6cf59a..53f428a4d2bd7d1a5c2dc0c64f7574d55c34073c 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -249,7 +249,7 @@ httpMaybeRemovePublic(StoreEntry * e, Http::StatusCode status)
     if (pe != NULL) {
         assert(e != pe);
 #if USE_HTCP
-        neighborsHtcpClear(e, NULL, e->mem_obj->request, e->mem_obj->method, HTCP_CLR_INVALIDATION);
+        neighborsHtcpClear(e, e->mem_obj->request, e->mem_obj->method, HTCP_CLR_INVALIDATION);
 #endif
         pe->release(true);
     }
@@ -266,7 +266,7 @@ httpMaybeRemovePublic(StoreEntry * e, Http::StatusCode status)
     if (pe != NULL) {
         assert(e != pe);
 #if USE_HTCP
-        neighborsHtcpClear(e, NULL, e->mem_obj->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_INVALIDATION);
+        neighborsHtcpClear(e, e->mem_obj->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_INVALIDATION);
 #endif
         pe->release(true);
     }

diff --git a/src/neighbors.cc b/src/neighbors.cc
index ed8e6164c214d5ac62e5a956086cf51466f9db9f..1cd9a4ced3d596745bf7c96962785510a5251eb1 100644 (file)
--- a/src/neighbors.cc
+++ b/src/neighbors.cc
@@ -1748,7 +1748,7 @@ neighborsHtcpReply(const cache_key * key, HtcpReplyData * htcp, const Ip::Addres
  * Send HTCP CLR messages to all peers configured to receive them.
  */
 void
-neighborsHtcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const HttpRequestMethod &method, htcp_clr_reason reason)
+neighborsHtcpClear(StoreEntry * e, HttpRequest * req, const HttpRequestMethod &method, htcp_clr_reason reason)
 {
     CachePeer *p;
     char buf[128];
@@ -1764,7 +1764,7 @@ neighborsHtcpClear(StoreEntry * e, const char *uri, HttpRequest * req, const Htt
             continue;
         }
         debugs(15, 3, "neighborsHtcpClear: sending CLR to " << p->in_addr.toUrl(buf, 128));
-        htcpClear(e, uri, req, method, p, reason);
+        htcpClear(e, req, method, p, reason);
     }
 }
 

diff --git a/src/neighbors.h b/src/neighbors.h
index a81ded1399842ec84933462153d4800b15a2b829..ada7dd9145ae4a3717503a7ddae2fb1dcd2a256b 100644 (file)
--- a/src/neighbors.h
+++ b/src/neighbors.h
@@ -39,7 +39,7 @@ void neighborsUdpAck(const cache_key *, icp_common_t *, const Ip::Address &);
 void neighborAdd(const char *, const char *, int, int, int, int, int);
 void neighbors_init(void);
 #if USE_HTCP
-void neighborsHtcpClear(StoreEntry *, const char *, HttpRequest *, const HttpRequestMethod &, htcp_clr_reason);
+void neighborsHtcpClear(StoreEntry *, HttpRequest *, const HttpRequestMethod &, htcp_clr_reason);
 #endif
 CachePeer *peerFindByName(const char *);
 CachePeer *peerFindByNameAndPort(const char *, unsigned short);

diff --git a/src/tests/stub_libanyp.cc b/src/tests/stub_libanyp.cc
index dceb726f6cdac625ae751662d6d65eff60a36d43..96c10af62d450bdfbb21bea13a7eaafe9e271d73 100644 (file)
--- a/src/tests/stub_libanyp.cc
+++ b/src/tests/stub_libanyp.cc
@@ -38,6 +38,5 @@ char *urlRInternal(const char *, unsigned short, const char *, const char *) STU
 char *urlInternal(const char *, const char *) STUB_RETVAL(nullptr)
 int matchDomainName(const char *, const char *, uint) STUB_RETVAL(0)
 int urlCheckRequest(const HttpRequest *) STUB_RETVAL(0)
-char *urlHostname(const char *) STUB_RETVAL(nullptr)
 void urlExtMethodConfigure() STUB
 

diff --git a/src/urn.cc b/src/urn.cc
index 9e34090d5892239a2a0e14dc657c7e36fb363cae..29c1dc676b2de534cdecb67146858bf038b1f805 100644 (file)
--- a/src/urn.cc
+++ b/src/urn.cc
@@ -370,7 +370,6 @@ urnParseReply(const char *inbuf, const HttpRequestMethod& m)
 {
     char *buf = xstrdup(inbuf);
     char *token;
-    char *host;
     url_entry *list;
     url_entry *old;
     int n = 32;
@@ -389,24 +388,23 @@ urnParseReply(const char *inbuf, const HttpRequestMethod& m)
             safe_free(old);
         }
 
-        host = urlHostname(token);
-
-        if (NULL == host)
+        AnyP::Uri uri;
+        if (!uri.parse(m, SBuf(token)) || !*uri.host())
             continue;
 
 #if USE_ICMP
-        list[i].rtt = netdbHostRtt(host);
+        list[i].rtt = netdbHostRtt(uri.host());
 
         if (0 == list[i].rtt) {
-            debugs(52, 3, "urnParseReply: Pinging " << host);
-            netdbPingSite(host);
+            debugs(52, 3, "Pinging " << uri.host());
+            netdbPingSite(uri.host());
         }
 #else
         list[i].rtt = 0;
 #endif
 
-        list[i].url = xstrdup(token);
-        list[i].host = xstrdup(host);
+        list[i].url = xstrdup(uri.absolute().c_str());
+        list[i].host = xstrdup(uri.host());
         // TODO: Use storeHas() or lock/unlock entry to avoid creating unlocked
         // ones.
         list[i].flags.cached = storeGetPublic(list[i].url, m) ? 1 : 0;