Re: [MERGE] RFC-compliant object invalidation behaviour.

On 28/08/2008, at 9:26 PM, Amos Jeffries wrote:

> Benno Rice wrote:

[snip]

> ------------------------------------------------------------------------
>>
>> # Bazaar merge directive format 2 (Bazaar 0.90)
>> # revision_id: benno_at_jeamland.net-20080828051217-kqnfk5s2huacfgn3
>> # target_branch: file:///home/benno/squid-work/squid3-repo/trunk/
>> # testament_sha1: e5013df20b582dbb9a4c9124644e6c521edd48ea
>> # timestamp: 2008-08-28 15:14:45 +1000
>> # base_revision_id: squid3_at_treenet.co.nz-20080827130136-\
>> # 58a98r50hgyj5e27
>> # # Begin patch
>> === modified file 'src/HttpRequestMethod.cc'
>> --- src/HttpRequestMethod.cc 2008-06-20 04:43:01 +0000
>> +++ src/HttpRequestMethod.cc 2008-08-28 05:12:17 +0000
>> @@ -246,7 +246,7 @@
>> /* all others */
>> case METHOD_OTHER:
>> default:
>> - return false; // be conservative: we do not know some
>> methods specs
>> + return true; // RFC says to purge if we don't know the
>> method
>
> Fair enough. But which RFC and section/subsection (maybe paragraph)?
> There is a unit-test needed for this function too.
> Ref: src/tests/testHttpRequestMethod.*

RFC 2616, 13.10, last paragraph. I've added a comment to this effect.

How do I run the unit test suite?

>> }
>> return false; // not reached
>> === modified file 'src/Server.cc'
>> --- src/Server.cc 2008-07-22 12:33:41 +0000
>> +++ src/Server.cc 2008-08-28 05:12:17 +0000
>> @@ -402,11 +402,20 @@
>> // purges entries that match the value of a given HTTP [response]
>> header
>> static void
>> -purgeEntriesByHeader(const char *reqUrl, HttpMsg *rep,
>> http_hdr_type hdr)
>> +purgeEntriesByHeader(const HttpRequest *req, const char *reqUrl,
>> HttpMsg *rep, http_hdr_type hdr)
>> {
>> - if (const char *url = rep->header.getStr(hdr))
>> - if (sameUrlHosts(reqUrl, url)) // prevent purging DoS, per
>> RFC 2616
>> + if (const char *url = rep->header.getStr(hdr)) {
>> + const char *absUrl = urlAbsolute(req, url);
>> + if (absUrl != NULL) {
>> + url = absUrl;
>> + }
>> + if (sameUrlHosts(reqUrl, url)) { // prevent purging DoS,
>> per RFC 2616
>> purgeEntriesByUrl(url);
>> + }
>
>> + if (absUrl != NULL) {
>> + xfree((void *)absUrl);
>> + }
>
> safe_free(absUrl);

Done.

>> + }
>> }
>> // some HTTP methods should purge matching cache entries
>> @@ -425,8 +434,8 @@
>> const char *reqUrl = urlCanonical(request);
>> debugs(88, 5, "maybe purging due to " <<
>> RequestMethodStr(request->method) << ' ' << reqUrl);
>> purgeEntriesByUrl(reqUrl);
>> - purgeEntriesByHeader(reqUrl, theFinalReply, HDR_LOCATION);
>> - purgeEntriesByHeader(reqUrl, theFinalReply,
>> HDR_CONTENT_LOCATION);
>> + purgeEntriesByHeader(request, reqUrl, theFinalReply,
>> HDR_LOCATION);
>> + purgeEntriesByHeader(request, reqUrl, theFinalReply,
>> HDR_CONTENT_LOCATION);
>> }
>> // called (usually by kids) when we have final (possibly adapted)
>> reply headers
>> === modified file 'src/protos.h'
>> --- src/protos.h 2008-07-17 12:38:06 +0000
>> +++ src/protos.h 2008-08-28 05:12:17 +0000
>> @@ -638,6 +638,7 @@
>> SQUIDCEXTERN void urlInitialize(void);
>> SQUIDCEXTERN HttpRequest *urlParse(const HttpRequestMethod&, char
>> *, HttpRequest *request = NULL);
>> SQUIDCEXTERN const char *urlCanonical(HttpRequest *);
>> +SQUIDCEXTERN const char *urlAbsolute(const HttpRequest *, const
>> char *);
>> SQUIDCEXTERN char *urlRInternal(const char *host, u_short port,
>> const char *dir, const char *name);
>> SQUIDCEXTERN char *urlInternal(const char *dir, const char *name);
>> SQUIDCEXTERN int matchDomainName(const char *host, const char
>> *domain);
>
> Most of the above should be in src/URL.h.
> Can you at least add the new ones there. The 'CEXTERN declaration is
> fine until the whole URL area gets cleaned up.

I think I'd rather leave it where it is until they all get moved,
otherwise it could cause confusion when it can't be found.

>> === modified file 'src/url.cc'
>> --- src/url.cc 2008-04-10 11:29:39 +0000
>> +++ src/url.cc 2008-08-28 05:12:17 +0000
>> @@ -532,6 +532,70 @@
>> return buf;
>> }
>>
>
> Now we get the bits I have not learned much of, so lessons may be in
> order.
>
>> +const char *
>> +urlAbsolute(const HttpRequest * req, const char *relUrl)
>> +{
>> + LOCAL_ARRAY(char, portbuf, 32);
>> + LOCAL_ARRAY(char, urlbuf, MAX_URL);
>
> LOCAL_ARRAY is unfortunately not thread safe. Someone else may
> argue, but I prefer to see them dead.

Replaced with stack-defined arrays.

>> + char *path, *last_slash;
>> +
>> + if (relUrl == NULL) {
>> + return (NULL);
>> + }
>> + if (req->method.id() == METHOD_CONNECT) {
>> + return (NULL);
>> + }
>> + if (strchr(relUrl, ':') != NULL) {
>> + return (NULL);
>
> Hmm, can relURL not contain the dynamic-part?
> I'd expect those to have ':' sometimes.

Hrm. I'll see if I can refine this. Any suggestions?

[snip]

>> + } else {
>> + portbuf[0] = '\0';
>> + if (req->port != urlDefaultPort(req->protocol)) {
>> + snprintf(portbuf, 32, ":%d", req->port);
>> + }
>> + if (relUrl[0] == '/') {
>> + snprintf(urlbuf, MAX_URL, "%s://%s%s%s%s%s",
>> + ProtocolStr[req->protocol],
>> + req->login,
>> + *req->login ? "@" : null_string,
>> + req->GetHost(),
>> + portbuf,
>> + relUrl
>> + );
>> + } else {
>> + path = xstrdup(req->urlpath.buf());
>
> Should be no need for that copy. Just construct the urlBuf
> incrementally and some ptr math when it comes to the last_slash
> memcpy().
> That would also remove the need for two different snprintf patterns
> below.

Incrementally? Do you mean as in using <<? Can you give me an
example? (C++ is not my strong suit =))

-- 
Benno Rice
benno_at_jeamland.net