Re: Refresh patterns and ACLs

2008-08-28 Thread Amos Jeffries

Mark Nottingham wrote:
One of the things that came up in Sydney briefly was whether the 
stale-while-revalidate and max-stale refresh_pattern options would be 
better expressed as ACLs.


Taking this a bit further, could/should the same be true of the rest of 
the refresh_pattern options (and perhaps of the patterns themselves)?


E.g., I have a user who wants to ignore-reload selectively based upon 
client IP; surely a good fit for an ACL if there ever were one...


Just food for thought,



Do you mean make a seperate tag? ie, send_stale [acl ...]

Um, I woudn't do all of the refresh_pattern options over, some actually 
do match the logical idea of affecting the keep/replace state of a 
cached object.


But the options that apply to active requests cached or not should be 
moved out to somewhere more understandable.


Amos
--
Please use Squid 2.7.STABLE4 or 3.0.STABLE8


Re: Regarding ESI

2008-08-28 Thread Amos Jeffries

Paras Fadte wrote:

Hi,

Can anybody please tell me from the below link what type is
parserState ? It is just below the  ParserState class .( it
has a comment  - /* todo factor this off somewhere else; */ )


 http://squid.treenet.co.nz/Doc/Code/ESIContext_8h-source.dyn



Sorry for the delay.

parserState variable in ESIContext class is publicly available and of 
type ESIContext::ParserState


I think the comment means the ParserState class should be extracted into 
an independant type at some future point?


Amos
--
Please use Squid 2.7.STABLE4 or 3.0.STABLE8


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

2008-08-28 Thread Benno Rice


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

[snip comments that I will act on shortly]

Just as a side question, once I've reworked my code to address Amos'  
concerns, what's the workflow for resubmitting?  Do I just bzr send  
again?  Is there anything I need to do to tell bundlebuggy (or  
whatever) that it's a resubmit?


--
Benno Rice
[EMAIL PROTECTED]





Re: cvs commit: squid/src url.c

2008-08-28 Thread Henrik Nordstrom
On tis, 2008-08-26 at 19:34 -0600, Benno Rice wrote:
 benno   2008/08/26 19:34:42 MDT
 
   Modified files:
 src  url.c 
   Log:
   Fix breakage caused by recent GCC fixes.
   
   The fixes in question prevented the returning of pre-defined method_t
   structures as well as any attempts at error reporting (and avoiding SEGVs) 
 by
   removing all checks against NULL.  If these checks are causing problems, 
 please
   consult with me about the best way to avoid them without messing up the 
 actual
   logic.

Sorry about the predefined method messup.

But the NULL checks after allocation is junk. The Squid xmalloc/xstrdup
functions can not return NULL (Squid aborts in such case, before
returning).

It can be discussed if this is good or bad, but it's the Squid-2 coding
standard, and adding checks here only adds overhead that will never
trigger and isn't seen anywhere else in the code..

Regards
Henrik


signature.asc
Description: This is a digitally signed message part


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

2008-08-28 Thread Henrik Nordstrom
On tor, 2008-08-28 at 23:26 +1200, Amos Jeffries wrote:

  +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.

I have always wanted to see them dead for many reasons.

* On reasonable systems (i.e. most non-dos) the stack is a very suitable
place for temporary allocations.

* Using a static local array as return value will bite you from time to
time even without threading, and is absolutely not thread safe..

  +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.

CONNECT URLs are a bit special.. but every other absolute URL must have
a scheme:

But relative URLs do not, ever..


  +}
  +if (req-protocol == PROTO_URN) {
  +   snprintf(urlbuf, MAX_URL, urn:%s, req-urlpath.buf());
 
 ?? no hostname or anything but path on URNs?
 Or is that a very badly named field?

URNs is differnt from URLs.. more like news: where the URI identifies a
resource as such and not where that something is located..

urn:isbn:0-596-00162-2

news:[EMAIL PROTECTED]

 Semantics of xstrdup() AFAIK already are that the recipient gets a 
 dynamic pointer and responsibility for its destruction.

Yes. xstrdup() is the same as strdup() except that it never returns NULL
and does not accept NULLs...

Related to this xstrndup() is like strndup with the rules above plus
that it always \0 terminates the result.

Regards
Henrik


signature.asc
Description: This is a digitally signed message part


Re: How to resubmit merge requests

2008-08-28 Thread Henrik Nordstrom
On tor, 2008-08-28 at 22:39 +1000, Benno Rice wrote:

 Just as a side question, once I've reworked my code to address Amos'  
 concerns, what's the workflow for resubmitting?  Do I just bzr send  
 again?  Is there anything I need to do to tell bundlebuggy (or  
 whatever) that it's a resubmit?

From what I have understood you should just do a bzr send again, and the
theory is that bundlebuggy then automatically detects that it's a
resubmission of the same branch and replaces/obsoletes the earlier
submission.

Regards
Henrik




signature.asc
Description: This is a digitally signed message part


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

2008-08-28 Thread Henrik Nordstrom
On tor, 2008-08-28 at 22:09 +0200, Henrik Nordstrom wrote:

 I have always wanted to see them dead for many reasons.
 
 * On reasonable systems (i.e. most non-dos) the stack is a very suitable
 place for temporary allocations.
 
 * Using a static local array as return value will bite you from time to
 time even without threading, and is absolutely not thread safe..

And forgot.. the current use of LOCAL_ARRAY for both temporary storage
and return values makes it hard to separate the two uses in an audit..

Regards
Henrik


signature.asc
Description: This is a digitally signed message part


Re: Refresh patterns and ACLs

2008-08-28 Thread Kinkie
 I vision a nested tree of matches (acl) and operators
 (allow/deny/refresh_pattern/outgoing_ip/tos/no-cache/ignore-xxx/deny_info/logmessage/peergroup/...).

 But it requires a different parser which is not single line oriented as
 you can not express a tree on a single line in a meaningful manner..


 request_access {
if [!]acls.. {
if [!]acls.. {
...
}
...
accept
}
deny
 }

YES please..
I'm quite familiar with the JunOS ACL format and it resembes this
pretty closely, it's very flexible..


-- 
 /kinkie


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

2008-08-28 Thread Kinkie
On Thu, Aug 28, 2008 at 10:09 PM, Henrik Nordstrom
[EMAIL PROTECTED] wrote:
 On tor, 2008-08-28 at 23:26 +1200, Amos Jeffries wrote:

  +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.

 I have always wanted to see them dead for many reasons.

 * On reasonable systems (i.e. most non-dos) the stack is a very suitable
 place for temporary allocations.

 * Using a static local array as return value will bite you from time to
 time even without threading, and is absolutely not thread safe..

Ironic.. this was exactly the reason why I started coding String-ng
(now temporarily renamed KBuf).


-- 
 /kinkie


Re: cvs commit: squid/src url.c

2008-08-28 Thread Benno Rice


On 29/08/2008, at 5:09 AM, Henrik Nordstrom wrote:


On tis, 2008-08-26 at 19:34 -0600, Benno Rice wrote:

benno   2008/08/26 19:34:42 MDT

 Modified files:
   src  url.c
 Log:
 Fix breakage caused by recent GCC fixes.

 The fixes in question prevented the returning of pre-defined  
method_t
 structures as well as any attempts at error reporting (and  
avoiding SEGVs) by
 removing all checks against NULL.  If these checks are causing  
problems, please
 consult with me about the best way to avoid them without messing  
up the actual

 logic.


Sorry about the predefined method messup.


No worries.

But the NULL checks after allocation is junk. The Squid xmalloc/ 
xstrdup

functions can not return NULL (Squid aborts in such case, before
returning).


Yeah, Adrian explained that to me.  I was just following my ingrained  
practice of always checking what malloc gives me and didn't realise  
that xmalloc had different behaviour.  I'll get rid of those checks  
shortly.


--
Benno Rice
[EMAIL PROTECTED]





Re: Refresh patterns and ACLs

2008-08-28 Thread Adrian Chadd
2008/8/29 Kinkie [EMAIL PROTECTED]:

 YES please..
 I'm quite familiar with the JunOS ACL format and it resembes this
 pretty closely, it's very flexible..

Make sure you can collapse those ACLs down to something sensible for
software processing before you go down that path!




Adrian


Re: Refresh patterns and ACLs

2008-08-28 Thread Mark Nottingham
I'm not convinced it's a great solution, but something like URISpace  
may be appropriate;

  http://www.w3.org/TR/urispace.html

What's nice about this is that you buy some efficiency by walking down  
the tree, rather than evaluating a linear set of rules...


Cheers,


On 29/08/2008, at 10:38 AM, Adrian Chadd wrote:


2008/8/29 Kinkie [EMAIL PROTECTED]:


YES please..
I'm quite familiar with the JunOS ACL format and it resembes this
pretty closely, it's very flexible..


Make sure you can collapse those ACLs down to something sensible for
software processing before you go down that path!




Adrian


--
Mark Nottingham   [EMAIL PROTECTED]




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

2008-08-28 Thread Amos Jeffries
 On tor, 2008-08-28 at 23:26 +1200, Amos Jeffries wrote:

  +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.

 I have always wanted to see them dead for many reasons.

 * On reasonable systems (i.e. most non-dos) the stack is a very suitable
 place for temporary allocations.

 * Using a static local array as return value will bite you from time to
 time even without threading, and is absolutely not thread safe..

  +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.

 CONNECT URLs are a bit special.. but every other absolute URL must have
 a scheme:

 But relative URLs do not, ever..

I know that, what I was querying was relative URL such as:
  /cgi-bin/parse.php?delim=:M

Which are valid, and would barf on that test.
Only the  , /, ;, and ? characters are required encoding by RFC 1738.



  +}
  +if (req-protocol == PROTO_URN) {
  +  snprintf(urlbuf, MAX_URL, urn:%s, req-urlpath.buf());

 ?? no hostname or anything but path on URNs?
 Or is that a very badly named field?

 URNs is differnt from URLs.. more like news: where the URI identifies a
 resource as such and not where that something is located..

 urn:isbn:0-596-00162-2

 news:[EMAIL PROTECTED]


Thanks Henrik. Here I have been thinking they were semantically URL, with
only a differing host part.

 Semantics of xstrdup() AFAIK already are that the recipient gets a
 dynamic pointer and responsibility for its destruction.

 Yes. xstrdup() is the same as strdup() except that it never returns NULL
 and does not accept NULLs...

 Related to this xstrndup() is like strndup with the rules above plus
 that it always \0 terminates the result.

 Regards
 Henrik





Re: How to resubmit merge requests

2008-08-28 Thread Amos Jeffries
 On tor, 2008-08-28 at 22:39 +1000, Benno Rice wrote:

 Just as a side question, once I've reworked my code to address Amos'
 concerns, what's the workflow for resubmitting?  Do I just bzr send
 again?  Is there anything I need to do to tell bundlebuggy (or
 whatever) that it's a resubmit?

 From what I have understood you should just do a bzr send again, and the
 theory is that bundlebuggy then automatically detects that it's a
 resubmission of the same branch and replaces/obsoletes the earlier
 submission.

It seems to do that for [MERGE] requests. But [PATCH] are seen as
independent.

Amos




Re: How to resubmit merge requests

2008-08-28 Thread Robert Collins
On Fri, 2008-08-29 at 13:13 +1200, Amos Jeffries wrote:
  On tor, 2008-08-28 at 22:39 +1000, Benno Rice wrote:
 
  Just as a side question, once I've reworked my code to address Amos'
  concerns, what's the workflow for resubmitting?  Do I just bzr send
  again?  Is there anything I need to do to tell bundlebuggy (or
  whatever) that it's a resubmit?
 
  From what I have understood you should just do a bzr send again, and the
  theory is that bundlebuggy then automatically detects that it's a
  resubmission of the same branch and replaces/obsoletes the earlier
  submission.
 
 It seems to do that for [MERGE] requests. But [PATCH] are seen as
 independent.

plain text patches can't be tracked; bundles can be. The mail topic is
irrelevant - bb looks at the content for tracking, the header only
determines if BB inspects the content or not.

-Rob
-- 
GPG key available at: http://www.robertcollins.net/keys.txt.


signature.asc
Description: This is a digitally signed message part


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

2008-08-28 Thread Benno Rice


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: [EMAIL PROTECTED]
# target_branch: file:///home/benno/squid-work/squid3-repo/trunk/
# testament_sha1: e5013df20b582dbb9a4c9124644e6c521edd48ea
# timestamp: 2008-08-28 15:14:45 +1000
# base_revision_id: [EMAIL PROTECTED]
#   58a98r50hgyj5e27
# # Begin patch
=== modified file 'src/HttpRequestMethod.cc'
--- src/HttpRequestMethod.cc2008-06-20 04:43:01 +
+++ src/HttpRequestMethod.cc2008-08-28 05:12:17 +
@@ -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 +
+++ src/Server.cc   2008-08-28 05:12:17 +
@@ -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.h2008-07-17 12:38:06 +
+++ src/protos.h2008-08-28 05:12:17 +
@@ -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 +
+++ src/url.cc  2008-08-28 05:12:17 +
@@ -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],
+   

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

2008-08-28 Thread Amos Jeffries

 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: [EMAIL PROTECTED]
 # target_branch: file:///home/benno/squid-work/squid3-repo/trunk/
 # testament_sha1: e5013df20b582dbb9a4c9124644e6c521edd48ea
 # timestamp: 2008-08-28 15:14:45 +1000
 # base_revision_id: [EMAIL PROTECTED]
 #   58a98r50hgyj5e27
 # # Begin patch
 === modified file 'src/HttpRequestMethod.cc'
 --- src/HttpRequestMethod.cc2008-06-20 04:43:01 +
 +++ src/HttpRequestMethod.cc2008-08-28 05:12:17 +
 @@ -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?

Single run is 'make check' after configuring. Then when you think its
works ./test-builds.sh for the full long run.


 }
  return false; // not reached
 === modified file 'src/Server.cc'
 --- src/Server.cc   2008-07-22 12:33:41 +
 +++ src/Server.cc   2008-08-28 05:12:17 +
 @@ -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.h2008-07-17 12:38:06 +
 +++ src/protos.h2008-08-28 05:12:17 +
 @@ -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 +
 +++ src/url.cc  2008-08-28 05:12:17 +
 @@ -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?


Just off the top of my head:

  X = strlen(relUrl); // or sourced some quicker way..
  p = relUrl;
  while(pX  *p  *p != ':'  *p != '?') p++;
  if(*p == ':') return 

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

2008-08-28 Thread Alex Rousskov

On Thu, 2008-08-28 at 15:16 +1000, Benno Rice wrote:
 RFC-compliant object invalidation behaviour.
   
 - Switch the default from not purging if the method is unknown to purging if
   the method is unknown.

 -  return false; // be conservative: we do not know some methods specs
 +  return true; // RFC says to purge if we don't know the method

Perhaps add that the RFC says SHOULD purge and refer to Section 13.10?
This requirement will probably surprise a few developers so it is better
to be explicit.

Also, if you change the default to return true, then we can delete all
return true cases in the same  HttpRequestMethod::purgesOthers()
method. This will make the method a tiny bit faster.

 - When purging URIs sourced from Location and Content-Location headers, make
   sure the URL is absolute before a) comparing it to see if hosts match and b)
   actually trying to find it in the store.
 
 These changes are based on changes made to squid 2.  In particular, the
 urlAbsolute function was ported from squid 2.  I would appreciate it if people
 paid particular attention to urlAbsolute to make sure I'm not doing anything
 that would cause problems in squid 3.

urlAbsolute() is not yet documented in the patch. Please add a few
comments to explain what it does and why we cannot use some of the
existing URL formatting routines [and then check their output] to arrive
at the answer.

Thank you,

Alex.





Re: Refresh patterns and ACLs

2008-08-28 Thread Tres Seaver
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Mark Nottingham wrote:
 I'm not convinced it's a great solution, but something like URISpace  
 may be appropriate;
http://www.w3.org/TR/urispace.html
 
 What's nice about this is that you buy some efficiency by walking down  
 the tree, rather than evaluating a linear set of rules...

Interesting spec:  I can see uses for it elsewhere.  A quick question,
(since grubbing around shows you to be the author;):  in section 3.3,
Path Segments, the semantics of path match=foo are to match the
next element in the current path, right?  Rather than matching any
random element (CSS style), or (for instance) the last element (which
would be useful in particular for the empty pattern and filename globs).

Is the spec frozen / dead, or could we suggest additions?  E.g.:

 path any=archives

and:

 path last=

I can certainly put such extensions into another namespace, but they
seem reasonably tightly connected to the existing first match semantics.


Tres.
- --
===
Tres Seaver  +1 540-429-0999  [EMAIL PROTECTED]
Palladion Software   Excellence by Designhttp://palladion.com
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIt3qO+gerLs4ltQ4RAiQoAJ9VCfO8pSohOB8ayLli3LAeymMHswCgiqV7
zrG+JVzw78PRioZqTCyL8T4=
=pQdp
-END PGP SIGNATURE-



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

2008-08-28 Thread Benno Rice


On 29/08/2008, at 2:13 PM, Alex Rousskov wrote:



On Thu, 2008-08-28 at 15:16 +1000, Benno Rice wrote:

RFC-compliant object invalidation behaviour.

- Switch the default from not purging if the method is unknown to  
purging if

 the method is unknown.


-  return false; // be conservative: we do not know some methods  
specs

+  return true; // RFC says to purge if we don't know the method


Perhaps add that the RFC says SHOULD purge and refer to Section 13.10?
This requirement will probably surprise a few developers so it is  
better

to be explicit.


Already done, as per Amos' request earlier in the thread.


Also, if you change the default to return true, then we can delete all
return true cases in the same  HttpRequestMethod::purgesOthers()
method. This will make the method a tiny bit faster.


That's true, although the gain will probably be minor as it would (I  
hope) be optimised that way.  It may also be worth leaving this layout  
in case it's desired to make this behaviour conditional.


- When purging URIs sourced from Location and Content-Location  
headers, make
 sure the URL is absolute before a) comparing it to see if hosts  
match and b)

 actually trying to find it in the store.

These changes are based on changes made to squid 2.  In particular,  
the
urlAbsolute function was ported from squid 2.  I would appreciate  
it if people
paid particular attention to urlAbsolute to make sure I'm not doing  
anything

that would cause problems in squid 3.


urlAbsolute() is not yet documented in the patch. Please add a few
comments to explain what it does and why we cannot use some of the
existing URL formatting routines [and then check their output] to  
arrive

at the answer.



Can do.  The idea is to form an absolute URL based on a provided  
relative URL and an exisiting record, eg a relative URL contained in  
the Content-Location header of a request.  If you know of a function  
that can do that, or if you can suggest a better way of doing this  
please let me know.  Otherwise I'll tidy the existing function up  
somewhat and add some comments describing its purpose.


--
Benno Rice
[EMAIL PROTECTED]