Re: Updated netfilter mark patch
On 20/09/10 00:41, Andrew Beverley wrote: I've moved it next to the headers check. I have also removed the error message that was generated if they don't exist. However, this means that if somebody explicitly sets --with-netfilter-conntrack and the libraries don't exist, then it will silently fail. Is this the behaviour we want? Hmm, we at least want to MSG_NOTICE for both cases, with preferrably a hard error if its explicitly stated. There'll be the default AC_SEARCH_LIBS notice in any case, and then it will also be shown later assuming --enable-zph-qos is set. I've just realised though that by default the QOS functions are disabled. I thought the new concept was that everything was enabled by default? Should I change the default to enabled for --enable-zph-qos? Yes please. Now enabled by default. I have also decided that (as previously suggested) a miss option would be useful in addition to the value preservation for a miss. This allows a miss value to be set when Squid has been compiled without libnetfilter-conntrack, and also makes it easier to set a miss value if you're happy with a consistent value. I have therefore added this into the latest patch (attached). The parameter is called 'miss' and it takes precedence over the preserve-miss feature. I've not heard back from my 2 testers yet, and I am yet to use it in anger myself, but I will give feedback on all these once I have it. Andy Any news? Unless someone has a reason not to I'm going to commit this when the current trunk build issues are resolved. Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.8 Beta testers wanted for 3.2.0.2
Re: Updated netfilter mark patch
On Mon, 2010-10-04 at 18:12 +1300, Amos Jeffries wrote: > On 20/09/10 00:41, Andrew Beverley wrote: > > I've moved it next to the headers check. I have also removed the error > > message that was generated if they don't exist. However, this means that > > if somebody explicitly sets --with-netfilter-conntrack and the libraries > > don't exist, then it will silently fail. Is this the behaviour we want? > > Hmm, we at least want to MSG_NOTICE for both cases, with preferrably a > hard error if its explicitly stated. > >>> > >>> There'll be the default AC_SEARCH_LIBS notice in any case, and then it > >>> will also be shown later assuming --enable-zph-qos is set. I've just > >>> realised though that by default the QOS functions are disabled. I > >>> thought the new concept was that everything was enabled by default? > >>> Should I change the default to enabled for --enable-zph-qos? > >> > >> Yes please. > > > > Now enabled by default. > > > > I have also decided that (as previously suggested) a miss option would > > be useful in addition to the value preservation for a miss. This allows > > a miss value to be set when Squid has been compiled without > > libnetfilter-conntrack, and also makes it easier to set a miss value if > > you're happy with a consistent value. I have therefore added this into > > the latest patch (attached). The parameter is called 'miss' and it takes > > precedence over the preserve-miss feature. > > > > I've not heard back from my 2 testers yet, and I am yet to use it in > > anger myself, but I will give feedback on all these once I have it. > > > > Andy > > > > Any news? > One of the testers has just now compiled a build with the patch in, so I should hear back from him shortly; the other I have not heard from. I am yet to use it in a production environment myself, but hope to do so soon. > Unless someone has a reason not to I'm going to commit this when the > current trunk build issues are resolved. I certainly wouldn't have any objections :)
Re: HTTP Compliance: What to do with Age:0 and IE5?
>> What do you recommend? > > Agreed: #1. Timing is about right with IE5 under 1% market share now and > still declining. > > Wrapping it in a custom macro (maybe "HTTP_VIOLATIONS && IE5_SUPPORT" ?) > that admin can set at build time would be better than 2. I agree with Amos. -- /kinkie
Re: [PATCH] HTTP Compliance: support requests with min-fresh
On 09/30/2010 06:15 AM, Amos Jeffries wrote: On 28/09/10 17:57, Alex Rousskov wrote: HTTP Compliance: support requests with Cache-Control: min-fresh. Added min-fresh directive support for Cache-Control header. The directive is handled in refreshCheck() by incrementing age and check_time by min-fresh value. Co-Advisor test case: test_case/rfc2616/ccReqDirMsg-min-fresh-obey +1. Committed to trunk (r10917). Could you also spare any time to check over bug 1202 and the patch there for relevance as a followup? Bug #1202 does not appear to be relevant here, but the patch there seems very useful on its own. I think we should use that patch as a starting point when adjusting our freshness algorithm to match that of HTTPbis (ticket #29, as pointed out by Mark Nottingham recently). Thank you, Alex.
Re: HTTP Compliance: What to do with Age:0 and IE5?
On 10/04/2010 06:26 AM, Kinkie wrote: What do you recommend? Agreed: #1. Timing is about right with IE5 under 1% market share now and still declining. Wrapping it in a custom macro (maybe "HTTP_VIOLATIONS&& IE5_SUPPORT" ?) that admin can set at build time would be better than 2. I agree with Amos. Committed #1 (i.e., removed the hack) to trunk as r10920. Thank you, Alex.
[PATCH] HTTP Compliance: Support If-Match and If-None-Match requests
HTTP Compliance: Support If-Match and If-None-Match requests.
Add support for If-Match and If-None-Match headers as described in RFC
2616 (sections 14.24 and 14.26 in particular).
Moved IMS handling from clientReplyContext::cacheHit() to
clientReplyContext::processConditional() while preserving the original
IMS logic, except for the case when a request has both IMS and
If-None-Match.
Co-Advisors test cases:
test_clause/rfc2616/ifMatch-mismatch-strong
test_clause/rfc2616/ifMatch-mismatch-weak
test_clause/rfc2616/ifNoneMatch-match-imsNone
and many more
HTTP Compliance: Support If-Match and If-None-Match requests.
Add support for If-Match and If-None-Match headers as described in RFC 2616
(sections 14.24 and 14.26 in particular).
Moved IMS handling from clientReplyContext::cacheHit() to
clientReplyContext::processConditional() while preserving the original IMS
logic, except for the case when a request has both IMS and If-None-Match.
Co-Advisors test cases:
test_clause/rfc2616/ifMatch-mismatch-strong
test_clause/rfc2616/ifMatch-mismatch-weak
test_clause/rfc2616/ifNoneMatch-match-imsNone
and many more
=== modified file 'errors/Makefile.am'
--- errors/Makefile.am 2010-08-23 02:21:19 +
+++ errors/Makefile.am 2010-10-01 16:49:56 +
@@ -19,40 +19,41 @@ ERROR_TEMPLATES = \
templates/ERR_CONNECT_FAIL \
templates/ERR_DIR_LISTING \
templates/ERR_DNS_FAIL \
templates/ERR_ESI \
templates/ERR_FORWARDING_DENIED \
templates/ERR_FTP_DISABLED \
templates/ERR_FTP_FAILURE \
templates/ERR_FTP_FORBIDDEN \
templates/ERR_FTP_NOT_FOUND \
templates/ERR_FTP_PUT_CREATED \
templates/ERR_FTP_PUT_ERROR \
templates/ERR_FTP_PUT_MODIFIED \
templates/ERR_FTP_UNAVAILABLE \
templates/ERR_ICAP_FAILURE \
templates/ERR_INVALID_REQ \
templates/ERR_INVALID_RESP \
templates/ERR_INVALID_URL \
templates/ERR_LIFETIME_EXP \
templates/ERR_NO_RELAY \
templates/ERR_ONLY_IF_CACHED_MISS \
+ templates/ERR_PRECONDITION_FAILED \
templates/ERR_READ_ERROR \
templates/ERR_READ_TIMEOUT \
templates/ERR_SECURE_CONNECT_FAIL \
templates/ERR_SHUTTING_DOWN \
templates/ERR_SOCKET_FAILURE \
templates/ERR_TOO_BIG \
templates/ERR_UNSUP_HTTPVERSION \
templates/ERR_UNSUP_REQ \
templates/ERR_URN_RESOLVE \
templates/ERR_WRITE_ERROR \
templates/ERR_ZERO_SIZE_OBJECT
TRANSLATE_LANGUAGES = \
af.lang \
ar.lang \
az.lang \
bg.lang \
ca.lang \
cs.lang \
da.lang \
=== modified file 'errors/list'
--- errors/list 2009-09-19 09:15:45 +
+++ errors/list 2010-10-01 16:49:56 +
@@ -5,31 +5,32 @@ ERR_CANNOT_FORWARD
ERR_CONNECT_FAIL
ERR_DIR_LISTING
ERR_DNS_FAIL
ERR_ESI
ERR_FORWARDING_DENIED
ERR_FTP_DISABLED
ERR_FTP_FAILURE
ERR_FTP_FORBIDDEN
ERR_FTP_NOT_FOUND
ERR_FTP_PUT_CREATED
ERR_FTP_PUT_ERROR
ERR_FTP_PUT_MODIFIED
ERR_FTP_UNAVAILABLE
ERR_ICAP_FAILURE
ERR_INVALID_REQ
ERR_INVALID_RESP
ERR_INVALID_URL
ERR_LIFETIME_EXP
ERR_NO_RELAY
ERR_ONLY_IF_CACHED_MISS
+ERR_PRECONDITION_FAILED
ERR_READ_ERROR
ERR_READ_TIMEOUT
ERR_SECURE_CONNECT_FAIL
ERR_SHUTTING_DOWN
ERR_SOCKET_FAILURE
ERR_TOO_BIG
ERR_UNSUP_HTTPVERSION
ERR_UNSUP_REQ
ERR_URN_RESOLVE
ERR_WRITE_ERROR
ERR_ZERO_SIZE_OBJECT
=== added file 'errors/templates/ERR_PRECONDITION_FAILED'
--- errors/templates/ERR_PRECONDITION_FAILED 1970-01-01 00:00:00 +
+++ errors/templates/ERR_PRECONDITION_FAILED 2010-10-01 17:04:18 +
@@ -0,0 +1,39 @@
+http://www.w3.org/TR/html4/strict.dtd";>
+
+
+ERROR: The requested URL could not be retrieved
+
+
+
+ERROR
+The requested URL could not be retrieved
+
+
+
+
+The following error was encountered while trying to retrieve the URL: %U
+
+
+Precondition Failed.
+
+
+This means:
+
+At least one precondition specified by the HTTP client in the request header has failed.
+
+
+
+
+
+
+
+Generated %T by %h (%s)
+
+
+
=== modified file 'src/HttpRequest.cc'
--- src/HttpRequest.cc 2010-09-12 00:10:47 +
+++ src/HttpRequest.cc 2010-10-01 16:49:56 +
@@ -557,40 +557,48 @@ HttpRequest::cacheable() const
* The below looks questionable: what non HTTP protocols use connect,
* trace, put and post? RC
*/
if (!method.isCacheble())
return false;
/*
* XXX POST may be cached sometimes.. ignored
* for now
*/
if (protocol == PROTO_GOPHER)
return gopherCachable(this);
if (protocol == PROTO_CACHEOBJ)
return false;
return true;
}
+bool
+HttpRequest::conditional() const
+{
+return flags.ims ||
+header.has(HDR_IF_MATCH) ||
+header.has(HDR_IF_NONE_MATCH);
+}
+
bool HttpRequest::inheritProperties(const HttpMsg *aMsg)
{
const HttpRequest* aReq = dynamic_cast(aMsg);
if (!aReq)
return false;
client_addr = aReq->client_addr;
#if FOLLOW_X_FORWARDED_FOR
indirec
Re: [AUDIT PATCH] ...
On 10/03/2010 07:31 PM, Amos Jeffries wrote: On Sun, 03 Oct 2010 19:16:35 -0600, Alex Rousskov wrote: What is the difference between [PATCH] and [AUDIT PATCH]? I thought I'd better do something to indicate it's not a working patch when committed alone. It's getting hard to weed out my list of [PATCH] emails pending the commit or audit fixes, even after deleting the ones that are superseded. If its annoying I'll drop the idea of extra words there Your call. I just do not understand the mnemonic. Since you are the one who is most likely to commit the patch, I would just mention the dependency in the cover email to squid-dev. I do hear your pain with pending changes interdependencies though. FWIW, I maintain project states elsewhere and only group/search squid-dev [PATCH] emails to track patch reviews and approvals (what bb use to do for us). Cheers, Alex.
Re: [AUDIT PATCH] comm cleanup - src/ipc/ changes
On 10/02/2010 10:06 AM, Amos Jeffries wrote:
Two patches:
Did not see the second patch attached. Was it send in a separate "pconn
changes" email?
patch #1:
This builds on the changes already audited in src/comm and src/base to
implement TCP listening ports being opened with ConnAcceptor and a
Subscription (to emit the HTTP or HTTPS client connect calls).
Alex; can you check that I have understood the IPC systems correctly?
+/// handler to subscribe to Comm::ConnAcceptor when we get the response
+Subscription::Pointer handlerSubscription;
It is not obvious what "response" we are talking about here. I would
remove the "when we ..." part or replace it with something like "after
we get the shared connection".
-int fd; ///< opened listening socket or -1
+Comm::ConnectionPointer conn; ///< opened listening socket or -1
The comment became stale. "Or nil"?
It is also weird that we are using Connection to represent a listening
socket that is not really connected to anything. Are you sure this is
the right change for now? Or should this fd->conn change wait until we
have a proper Descriptor class perhaps?
+if (sock_type == SOCK_STREAM) {
+// TCP: setup the subscriptions such that new connections accepted by
listenConn are handled by HTTP
+AsyncJob::Start(new Comm::ConnAcceptor(cbd->conn, FdNote(fdNote),
sub));
+} else if (sock_type == SOCK_DGRAM) {
+// UDP: setup the listener socket, but do not set a subscriber
+Comm::ConnectionPointer udpConn = listenConn;
+comm_open_listener(sock_type, proto, udpConn, FdNote(fdNote));
+} else {
+fatalf("Invalid Socket Type (%d)",sock_type);
+}
This is out of place in Ipc::StartListening(), IMO. Ipc should not know
or care what kind of listener is being setup. Listener-specific
discrimination should be done at a higher level or, at the very least,
at Comm level. Ipc just shovels the descriptors to and from.
Please also note that the StartListening() description does not mention
that SOCK_DGRAM and SOCK_STREAM (and "other") are handled very differently.
+if (sock_type == SOCK_STREAM) {
+// TCP: setup the subscriptions such that new connections accepted by
listenConn are handled by HTTP
+AsyncJob::Start(new Comm::ConnAcceptor(cbd->conn, FdNote(fdNote),
sub));
+}
...
+cbd->errNo = cbd->conn->isOpen() ? 0 : errno;
Will cbd->conn->isOpen() always be true for SOCK_STREAM here? If not,
then errno should not be used. If yes, perhaps we do not need to set
cbd->errNo in the SOCK_STREAM context?
=== modified file 'src/ipc/Strand.cc'
--- src/ipc/Strand.cc 2010-07-06 23:09:44 +
+++ src/ipc/Strand.cc 2010-09-24 09:29:47 +
@@ -6,13 +6,14 @@
*/
#include "config.h"
+#include "base/Subscription.h"
#include "base/TextException.h"
+#include "comm/Connection.h"
#include "ipc/Strand.h"
#include "ipc/Messages.h"
#include "ipc/SharedListen.h"
#include "ipc/Kids.h"
Are all of the above changes needed even though there are no other
changes in src/ipc/Strand.cc?
>
> -
> CBDATA_NAMESPACED_CLASS_INIT(Ipc, Strand);
>
-
-
Please avoid unnecessary whitespace changes in functionality patches.
They just increase the probability of a conflict with other pending patches.
-options(COMM_NONBLOCKING),
-fd_(-1)
+options(COMM_NONBLOCKING)
Sometimes you replace FD initialization with connection(NULL)
initialization and sometimes you do not. Please be consistent.
-int fd(); ///< creates if needed and returns raw UDS socket descriptor
+Comm::ConnectionPointer &conn(); ///< creates if needed and returns raw
UDS socket descriptor
...
-int fd_; ///< UDS descriptor
+Comm::ConnectionPointer conn_; ///< UDS descriptor
Stale "descriptor is not a connection" comment and the
descriptor-vs-connection problem again.
Thank you,
Alex.
Re: [AUDIT PATCH] comm cleanup - pconn changes
On 10/02/2010 10:55 AM, Amos Jeffries wrote:
Implements the pconn and IdleConnList as storage lists of
Comm::Connection.
With Comm::Connection the key definition has to change. The old keying
based solely on domain would produce a Connection object whose FD did
not exactly match the remote IP:port selected by the new routing logics.
I've kept the concept that the pconn key is the details to describe the
remote end of the link used to fetch any given domain. As such its
become much simpler now. Just the remote IP:port from the TCP details
and requested domain (post re-writing) or peer hostname for peer pconn.
The change also removes the client address from relevance (enhancement
bug fix). Instead we scan the list of pconn going to our desired
remote-end for the local-end details (tcp_outgoing_addr or tproxy IPs)
to pick an exact pconn match (bug fix).
.
-list->removeFD(fd); /* might delete list */
-comm_close(fd);
+/* might delete list */
+if (list && list->remove(conn)) {
+Comm::ConnectionPointer nonConst = conn;
+nonConst->close();
+}
The list member cannot be NULL until the remove is called. The nonConst
conversion is also weird but I will discuss that later. Should probably
be rewritten as:
Comm::ConnectionPointer deletedConn =
list->remove(conn); // might delete list
if (deletedConn != NULL)
deletedConn->close();
However, this is still not equivalent to the old code because the old
code was closing the connection regardless of whether it was found in
the list. Is that an intentional change? Perhaps add a comment about it?
-list->removeFD(fd); /* might delete list */
-comm_close(fd);
+Comm::ConnectionPointer temp = new Comm::Connection; // XXX: transition.
make timeouts pass conn in
+temp->fd = fd;
+if (list->remove(temp)) {
+temp->close();
+} else
+temp->fd = -1; // XXX: transition. prevent temp erasure double-closing
FD until timeout CB passess conn in.
Lost that critical, IMO, "might delete list" comment.
Also, the creation of a temporary connection seems unnecessary when we
have a findIndex method. Use findIndex or, better, add a findConn method
if you want to close the found connection.
The same concerns about not-closing of the not-found connection apply
here as well.
+for (int index = 0; index < nfds; index++)
+theList[index] = oldList[index];
This makes IdleConnList::push slower that the old code that used
xmemcpy(). The shifting overhead is one of the reasons we should not use
[C] arrays here, IMO.
-IdleConnList::read(int fd, char *buf, size_t len, comm_err_t flag, int xerrno,
void *data)
+IdleConnList::read(const Comm::ConnectionPointer &conn, char *buf, size_t len,
comm_err_t flag, int xerrno, void *data)
Please capitalize static Read() if you are changing its profile anyway.
+int used = strlen(buf);
Please use const.
+Comm::ConnectionPointer nonConst = conn;
+Comm::ConnectionPointer nonConst = conn;
+Comm::ConnectionPointer nonConst = conn;
...
This needs to be fixed somehow as it is only going to get worse. There
are two questions here:
1) Does "const Pointer" always mean that we cannot change the class the
Pointer points to? No, it does not. It only means that we can never
change the Pointer itself. Our RefCount is rather confused about
const-correctness, but we can (and should) fix that.
2) Does Comm have a right to change Connection or similar classes
submitted to it for I/O management? Here, the decision is up to us, but
I think it is pretty clear that a lot of our Comm code already fiddles
with FD state (via fd_table and such) and, hence, with the Connection state.
If you agree, then calling conn->close() for "const Connection::Pointer
&conn" passed to a Comm I/O handler is just fine. If you need help
fixing RefCount to support that, please let me know. Otherwise, just
follow auto_ptr or any semi-standard smart pointer layout.
+/**
+ * Search the list for an existing connection. Matches by FD.
+ *
+ * \retval false The connection does not currently exist in the list.
+ *We seem to have hit and lost a race condition.
+ *Nevermind, but MUST NOT do anything with the raw FD.
+ */
+bool remove(const Comm::ConnectionPointer &conn);
You forgot to mention that remove() removes :-).
As discussed above, the "MUST NOT do anything" caveat seems to
contradict the current code which does something. Can you describe a
race condition that would make the Comm callback state essentially
invalid? Perhaps we can avoid spreading this FUD throughout the handlers
code...
+/**
+ * Updates destLink to point at an existing open connection if available
and retriable.
+ * Otherwise, return false.
+ *
+ * We close available persistent connection if the caller transaction is
not
+
Re: [PATCH] HTTP Compliance: Support If-Match and If-None-Match requests
On Mon, 04 Oct 2010 10:22:37 -0600, Alex Rousskov wrote: > HTTP Compliance: Support If-Match and If-None-Match requests. > > Add support for If-Match and If-None-Match headers as described in RFC > 2616 (sections 14.24 and 14.26 in particular). > > Moved IMS handling from clientReplyContext::cacheHit() to > clientReplyContext::processConditional() while preserving the original > IMS logic, except for the case when a request has both IMS and > If-None-Match. > > Co-Advisors test cases: > test_clause/rfc2616/ifMatch-mismatch-strong > test_clause/rfc2616/ifMatch-mismatch-weak > test_clause/rfc2616/ifNoneMatch-match-imsNone > and many more +1. Yay. Please close bug 427 with this commit. Amos
HTTP Compliance: entry is stale if request has max-age=0
HTTP Compliance: entry is stale if request has max-age=0.
We should always do validation for requests with Cache-Control
max-age=0, even when entry age is also zero. In our case, RFC 2616 says:
freshness_lifetime = max_age_value
response_is_fresh = (freshness_lifetime > current_age)
response_is_fresh is always false if freshness_lifetime is zero...
The check code was introduced in r5998 with a "Import of fix-ranges
branch" message. The code was commented out at the time of that commit,
for reasons unknown.
Test case:
test_case/rfc2616/noSrv-hit-stale-max-age-req
HTTP Compliance: entry is stale if request has max-age=0.
We should always do validation for requests with Cache-Control max-age=0,
even when entry age is also zero. In our case, RFC 2616 says:
freshness_lifetime = max_age_value
response_is_fresh = (freshness_lifetime > current_age)
and response_is_fresh is always false if freshness_lifetime is zero.
The check code was introduced in r5998 with a "Import of fix-ranges
branch" message. The code was commented out at the time of that
commit, for reasons unknown.
Test case:
test_case/rfc2616/noSrv-hit-stale-max-age-req
=== modified file 'src/refresh.cc'
--- src/refresh.cc 2010-05-14 09:47:03 +
+++ src/refresh.cc 2010-09-16 02:34:29 +
@@ -298,48 +298,45 @@ refreshCheck(const StoreEntry * entry, H
/* The clients no-cache header is ignored */
debugs(22, 3, "refreshCheck: MAYBE: ignore-reload");
} else if (R->flags.reload_into_ims || Config.onoff.reload_into_ims) {
/* The clients no-cache header is changed into a IMS query */
debugs(22, 3, "refreshCheck: YES: reload-into-ims");
return STALE_RELOAD_INTO_IMS;
} else {
/* The clients no-cache header is not overridden on this request */
debugs(22, 3, "refreshCheck: YES: client reload");
request->flags.nocache = 1;
return STALE_FORCED_RELOAD;
}
#endif
if (NULL != cc) {
if (cc->max_age > -1) {
#if USE_HTTP_VIOLATIONS
if (R->flags.ignore_reload && cc->max_age == 0) {} else
#endif
{
-#if 0
-
if (cc->max_age == 0) {
debugs(22, 3, "refreshCheck: YES: client-max-age = 0");
return STALE_EXCEEDS_REQUEST_MAX_AGE_VALUE;
}
-#endif
if (age > cc->max_age) {
debugs(22, 3, "refreshCheck: YES: age > client-max-age");
return STALE_EXCEEDS_REQUEST_MAX_AGE_VALUE;
}
}
}
if (EBIT_TEST(cc->mask, CC_MAX_STALE) && staleness > -1) {
if (cc->max_stale < 0) {
/* max-stale directive without a value */
debugs(22, 3, "refreshCheck: NO: max-stale wildcard");
return FRESH_REQUEST_MAX_STALE_ALL;
} else if (staleness < cc->max_stale) {
debugs(22, 3, "refreshCheck: NO: staleness < max-stale");
return FRESH_REQUEST_MAX_STALE_VALUE;
}
}
}
}
Re: [AUDIT PATCH] comm cleanup - src/ipc/ changes
On Mon, 04 Oct 2010 12:46:07 -0600, Alex Rousskov
wrote:
> On 10/02/2010 10:06 AM, Amos Jeffries wrote:
>
>> Two patches:
>
> Did not see the second patch attached. Was it send in a separate "pconn
> changes" email?
Yes sorry. I found a bug before attaching and forgot to update the text.
>
>
>> patch #1:
>> This builds on the changes already audited in src/comm and src/base to
>> implement TCP listening ports being opened with ConnAcceptor and a
>> Subscription (to emit the HTTP or HTTPS client connect calls).
>>
>> Alex; can you check that I have understood the IPC systems correctly?
>
>> +/// handler to subscribe to Comm::ConnAcceptor when we get the
>> response
>> +Subscription::Pointer handlerSubscription;
>
> It is not obvious what "response" we are talking about here. I would
> remove the "when we ..." part or replace it with something like "after
> we get the shared connection".
>
>> -int fd; ///< opened listening socket or -1
>> +Comm::ConnectionPointer conn; ///< opened listening socket or -1
>
> The comment became stale. "Or nil"?
>
Okay. Fixing.
> It is also weird that we are using Connection to represent a listening
> socket that is not really connected to anything. Are you sure this is
> the right change for now? Or should this fd->conn change wait until we
> have a proper Descriptor class perhaps?
The listening socket has all the same fields and connection management
requirements as any other read-only socket, but remote IP being nil.
>
>> +if (sock_type == SOCK_STREAM) {
>> +// TCP: setup the subscriptions such that new connections
>> accepted by listenConn are handled by HTTP
>> +AsyncJob::Start(new Comm::ConnAcceptor(cbd->conn,
>> FdNote(fdNote), sub));
>> +} else if (sock_type == SOCK_DGRAM) {
>> +// UDP: setup the listener socket, but do not set a subscriber
>> +Comm::ConnectionPointer udpConn = listenConn;
>> +comm_open_listener(sock_type, proto, udpConn, FdNote(fdNote));
>> +} else {
>> +fatalf("Invalid Socket Type (%d)",sock_type);
>> +}
>
> This is out of place in Ipc::StartListening(), IMO. Ipc should not know
> or care what kind of listener is being setup. Listener-specific
> discrimination should be done at a higher level or, at the very least,
> at Comm level. Ipc just shovels the descriptors to and from.
I was a little worried about this. The problem is that we don't yet have a
UDP acceptor/listener/reader object. and I don't want to bloat this commit
any more than I have to. ConnAcceptor is for TCP accept() handshakes only
so to spawn them from IPC this if() is required on one form or another.
So the question is whether:
#1) IPC should be spawning ConnAcceptor and the UDP class (in which case
the if stays) since they are not needed until the FD has arrived, or...
#2) if their first action should be for the caller to create the relevant
ConnAcceptor/UDP themselves, subscribe. With ConnAcceptor/UDS running IPC
in their start() to fetch the FD
I picked (1) for the submission patch since you had IPC calling
comm_open_listener() in trunk.
>
> Please also note that the StartListening() description does not mention
> that SOCK_DGRAM and SOCK_STREAM (and "other") are handled very
differently.
>
They are not very different. Only the "other" case with its fatal().
AsyncJob::Start(new Comm::ConnAcceptor(...)) is the wrapper of
comm_open_listener(SOCK_STREAM, ...). IPC is then finished and returns the
listening-done call. The big difference is in the followup async steps
outside of IPC.
Outside of IPC;
the UDP listen-done handles the packet.
the TCP listen-done waits for any subscription calls to arrive.
>
>> +if (sock_type == SOCK_STREAM) {
>> +// TCP: setup the subscriptions such that new connections
>> accepted by listenConn are handled by HTTP
>> +AsyncJob::Start(new Comm::ConnAcceptor(cbd->conn,
>> FdNote(fdNote), sub));
>> +}
> ...
>> +cbd->errNo = cbd->conn->isOpen() ? 0 : errno;
>
> Will cbd->conn->isOpen() always be true for SOCK_STREAM here? If not,
> then errno should not be used. If yes, perhaps we do not need to set
> cbd->errNo in the SOCK_STREAM context?
Not.
If ConnAcceptor::start(){comm_open_listener(){comm_openex()}} fails for
any reason it will be false.
>
>> === modified file 'src/ipc/Strand.cc'
>> --- src/ipc/Strand.cc2010-07-06 23:09:44 +
>> +++ src/ipc/Strand.cc2010-09-24 09:29:47 +
>> @@ -6,13 +6,14 @@
>> */
>>
>> #include "config.h"
>> +#include "base/Subscription.h"
>> #include "base/TextException.h"
>> +#include "comm/Connection.h"
>> #include "ipc/Strand.h"
>> #include "ipc/Messages.h"
>> #include "ipc/SharedListen.h"
>> #include "ipc/Kids.h"
>
>
> Are all of the above changes needed even though there are no other
> changes in src/ipc/Strand.cc?
Yes. gcc informs that Strand.cc is instantiating a template (SharedListen
IIRC) which now has both of the above as dependenci
Re: [AUDIT PATCH] comm cleanup - pconn changes
On Mon, 04 Oct 2010 14:31:25 -0600, Alex Rousskov
wrote:
> On 10/02/2010 10:55 AM, Amos Jeffries wrote:
>> Implements the pconn and IdleConnList as storage lists of
>> Comm::Connection.
>>
>> With Comm::Connection the key definition has to change. The old keying
>> based solely on domain would produce a Connection object whose FD did
>> not exactly match the remote IP:port selected by the new routing
logics.
>>
>> I've kept the concept that the pconn key is the details to describe the
>> remote end of the link used to fetch any given domain. As such its
>> become much simpler now. Just the remote IP:port from the TCP details
>> and requested domain (post re-writing) or peer hostname for peer pconn.
>>
>> The change also removes the client address from relevance (enhancement
>> bug fix). Instead we scan the list of pconn going to our desired
>> remote-end for the local-end details (tcp_outgoing_addr or tproxy IPs)
>> to pick an exact pconn match (bug fix).
>
> .
>
>> -list->removeFD(fd); /* might delete list */
>> -comm_close(fd);
>> +/* might delete list */
>> +if (list && list->remove(conn)) {
>> +Comm::ConnectionPointer nonConst = conn;
>> +nonConst->close();
>> +}
>
> The list member cannot be NULL until the remove is called. The nonConst
> conversion is also weird but I will discuss that later. Should probably
> be rewritten as:
>
> Comm::ConnectionPointer deletedConn =
> list->remove(conn); // might delete list
> if (deletedConn != NULL)
> deletedConn->close();
>
> However, this is still not equivalent to the old code because the old
> code was closing the connection regardless of whether it was found in
> the list. Is that an intentional change? Perhaps add a comment about it?
>
>> -list->removeFD(fd); /* might delete list */
>> -comm_close(fd);
>> +Comm::ConnectionPointer temp = new Comm::Connection; // XXX:
>> transition. make timeouts pass conn in
>> +temp->fd = fd;
>> +if (list->remove(temp)) {
>> +temp->close();
>> +} else
>> +temp->fd = -1; // XXX: transition. prevent temp erasure
>> double-closing FD until timeout CB passess conn in.
>
> Lost that critical, IMO, "might delete list" comment.
>
> Also, the creation of a temporary connection seems unnecessary when we
> have a findIndex method. Use findIndex or, better, add a findConn method
> if you want to close the found connection.
>
> The same concerns about not-closing of the not-found connection apply
> here as well.
>
>
>> +for (int index = 0; index < nfds; index++)
>> +theList[index] = oldList[index];
>
> This makes IdleConnList::push slower that the old code that used
> xmemcpy(). The shifting overhead is one of the reasons we should not use
> [C] arrays here, IMO.
>
>
>> -IdleConnList::read(int fd, char *buf, size_t len, comm_err_t flag, int
>> xerrno, void *data)
>> +IdleConnList::read(const Comm::ConnectionPointer &conn, char *buf,
>> size_t len, comm_err_t flag, int xerrno, void *data)
>
> Please capitalize static Read() if you are changing its profile anyway.
>
>
>> +int used = strlen(buf);
>
> Please use const.
>
>
>> +Comm::ConnectionPointer nonConst = conn;
>> +Comm::ConnectionPointer nonConst = conn;
>> +Comm::ConnectionPointer nonConst = conn;
> ...
>
> This needs to be fixed somehow as it is only going to get worse. There
> are two questions here:
>
> 1) Does "const Pointer" always mean that we cannot change the class the
> Pointer points to? No, it does not. It only means that we can never
> change the Pointer itself. Our RefCount is rather confused about
> const-correctness, but we can (and should) fix that.
>
> 2) Does Comm have a right to change Connection or similar classes
> submitted to it for I/O management? Here, the decision is up to us, but
> I think it is pretty clear that a lot of our Comm code already fiddles
> with FD state (via fd_table and such) and, hence, with the Connection
> state.
>
> If you agree, then calling conn->close() for "const Connection::Pointer
> &conn" passed to a Comm I/O handler is just fine. If you need help
> fixing RefCount to support that, please let me know. Otherwise, just
> follow auto_ptr or any semi-standard smart pointer layout.
>
Completely agree. All my uses of "nonConst" are to call ->close() past
that brain-damage.
>
>> +/**
>> + * Search the list for an existing connection. Matches by FD.
>> + *
>> + * \retval false The connection does not currently exist in the
>> list.
>> + *We seem to have hit and lost a race condition.
>> + *Nevermind, but MUST NOT do anything with the raw
>> FD.
>> + */
>> +bool remove(const Comm::ConnectionPointer &conn);
>
> You forgot to mention that remove() removes :-).
>
> As discussed above, the "MUST NOT do anything" caveat seems to
> contradict the current code which d
Re: HTTP Compliance: entry is stale if request has max-age= 0
On Mon, 04 Oct 2010 17:06:28 -0600, Alex Rousskov wrote: > HTTP Compliance: entry is stale if request has max-age=0. > > We should always do validation for requests with Cache-Control > max-age=0, even when entry age is also zero. In our case, RFC 2616 says: > > freshness_lifetime = max_age_value > response_is_fresh = (freshness_lifetime > current_age) > > response_is_fresh is always false if freshness_lifetime is zero... > > > The check code was introduced in r5998 with a "Import of fix-ranges > branch" message. The code was commented out at the time of that commit, > for reasons unknown. > > Test case: > test_case/rfc2616/noSrv-hit-stale-max-age-req +1 on the patch. In other related bits; I think we have a documentation bug with ignore-reload. That violations if() case above it silently tests and skips for ignore-reload && max-age=0. Could you please add a debugs() message to the if() case matching the other trace cases? The test itself looks wrong to me, I would expect max-age=* all equate to a client reload request when the object is outside that age. In these cases it's unclear if a reload should be allowed despite the admin preference (as now done). Or if the client should receive a fail or stale response. My leaning is that the client should get the stale response. The admin has explicitly required a violation of HTTP and chosen to face the consequences. In 3.1+ reverse-proxies the http_port "ignore-cc" option should be used instead. Amos
Re: [PATCH] AsyncCall and CommCalls changes for cleanup-comm work.
On 09/23/2010 10:05 AM, Amos Jeffries wrote:
Surprisingly small. These are the changes to src/base and CommCalls.*
where all the Call related code has been changed.
Including the generalized Subscription mechanism used by ConnAcceptor.
CommCalls.* is still a work in progress. I'm confidant that much more of
the code there can be removed as the cleanup progresses in the other
components to make them use AsyncCalls more generically.
The changes visible there are indicative of most of the changes to the
wider squid code.
Alex: removing the XXX marked const problems and testing they will work
is blocked behind some build errors from the IOCB definition change. As
soon as I have building code that wont hide const side-effects I'll be
removing that muckup.
+typedef void IOACB(int fd, const Comm::ConnectionPointer &details, comm_err_t
flag, int xerrno, void *data);
+typedef void CNCB(const Comm::ConnectionPointer &conn, comm_err_t status, int
xerrno, void *data);
+typedef void IOCB(const Comm::ConnectionPointer &conn, char *, size_t size,
comm_err_t flag, int xerrno, void *data);
It would be nice if each of the three names is deciphered in lieu of
documentation for the types we are defining here.
public:
void *data; // cbdata-protected
-int fd;
+Comm::ConnectionPointer conn;
+int fd; // raw FD from legacy calls. use conn instead.
Having both fd and conn is bad, but having them pretty much undocumented
makes it impossible for me to review the patch: I cannot review the
sometimes complex interactions between the two in the rest of the patch
because I do not understand when we should have none, one, the other,
and both members. Did I miss some documentation piece explaining this?
@@ -65,12 +69,6 @@
{
public:
CommAcceptCbParams(void *aData);
-
-void print(std::ostream &os) const;
-
-public:
-ConnectionDetail details;
-int nfd; // TODO: rename to fdNew or somesuch
};
Why was nfd removed? Is it not specific to the accept callback?
// connect parameters
@@ -80,11 +78,6 @@
CommConnectCbParams(void *aData);
bool syncWithComm(); // see CommCommonCbParams::syncWithComm
-
-void print(std::ostream &os) const;
-
-public:
-DnsLookupDetails dns;
};
Where did the dns details go? Are they now supplied using some other API?
// read/write (I/O) parameters
@@ -176,10 +169,12 @@
{
public:
typedef CommAcceptCbParams Params;
+typedef RefCount Pointer;
CommAcceptCbPtrFun(IOACB *aHandler, const CommAcceptCbParams &aParams);
+CommAcceptCbPtrFun(const CommAcceptCbPtrFun &o);
+
.
void dial();
-
Please avoid whitespace changes in functionality-focused patches.
+inline CommCbFunPtrCallT(const Pointer &p) :
The "inline" keyword is not needed if you are inlining directly after
the declaration.
+AsyncCall(p->debugSection, p->debugLevel, p->name),
+dialer(p->dialer)
+{}
+
.
=== modified file 'src/base/AsyncCall.cc'
--- src/base/AsyncCall.cc 2009-04-09 22:31:13 +
+++ src/base/AsyncCall.cc 2010-09-10 12:36:02 +
@@ -77,4 +77,3 @@
AsyncCallQueue::Instance().schedule(call);
return true;
}
-
Please avoid whitespace changes in functionality-focused patches.
I will comment on the Subscription class separately.
Thank you,
Alex.
