Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 18/01/2013 10:23 a.m., Alex Rousskov wrote: On 01/17/2013 10:56 AM, Rainer Weikusat wrote: Alex Rousskov writes: On 01/16/2013 09:58 PM, Amos Jeffries wrote: * timeout() happens first and runs doneConnecting() while connect() is queued. Yes (timeout wins and ends ConnOpener job). It is possible to 'do better' here fairly easily, see more detailed explanation below. No, it is not. It is possible to spend a lot of time on writing, reviewing, and fixing complicated code that will optimize the case that virtually never happens. Correctly detecting this race and properly handling it is not easy and is not useful. - we have no handle to the connect() AsyncCall stored so we can't cancel it directly (bug?). We do not need to cancel it (just like with the timeout call discussed above). The async call will not be delivered to the destroyed ConnOpener job). I'm rather in favour of cancelling the calls, at least where easily possible, because this makes the debugging/ diagnostic output more easy to understand. We should, of course, clear the stored timeout (or I/O) callback after that callback has been fired. Without that, swanSong() would try to clear a fired callback, and that would be confusing. I do not have the guts to object to us calling the cancel() method for the non-fired callback that is no longer relevant (as long as it is done consistently). However, I do not see how that improves debugging in ConnOpener case because the callback will know that the job is gone without our explicit cancellation. And I am sure somebody will eventually think that calling cancel has some useful side-effects such as closing file descriptors or freeing some memory (it does not). Ending the job is enough to guarantee that it will not be called via an async job call: Taking this into account and after having spent some more time thinking on this, I think what the timeout handler should do is, 1. Clear calls_.timeout_. Yes. calls.timeout must reflect whether there is a timeout notification scheduled (from our job point of view). Yes. 2. Check if temporaryFd_ is still valid. At the moment, this is always the case, but it would prudent wrt general 'defensive programming' and the check will become necessary for change 3|. I do not know what valid is in this context, but temporaryFd_ and earlyAbort must reflect whether there is a Comm I/O scheduled (from our job point of view). Valid is (temporaryFd_ >= 0). Unless that is true there is no connect operation in-progress, the Job is either setting up still or closing down already. The doneAll() indicates whether closing down is underway or will happen next. When the connection is done successfully temporaryFd_ is moved to conn, on failure temporaryFd_ is cleaned up directly. This is all handled by doneConnecting(). 2a. If this was the case, check if the write handler of the corresponding fde is still set. If I/O is scheduled, we should stop it, close the descriptor, and free memory using Comm APIs. This condition should probably be detected only in swanSong(), to avoid code duplication. The job should exit after a timeout, of course. Accessing Comm write handler pointers in some low-level table should be avoided if at all possible. If Comm APIs need to be adjusted to handle this better, we can adjust them. Amos, do you remember what "partially open FD" in the following comment meant, exactly? Why cannot we just call comm_close() to cleanup? It means we have issued socket() and connect() OS calls. fd_table[] state probably exists. But we are still waiting on either TCP response, or the In-Progress event to run and the FD to be confirmed as open. This is signalled by (temporaryFd_ >= 0). // it never reached fully open, so cleanup the FD handlers // Note that comm_close() sequence does not happen for partially open FD In your latest patch, please move fd cleanup from timeout() and from doneConnecting() into a dedicated method and call that method from swanSong() if temporaryFd_ is still set there. Do not call it from doneConnecting() or anywhere else. Remember that a job may end for reasons other than some timeout or I/O. swanSong() is the cleanup method. Please do not forget to set calls.earlyAbort to NULL after canceling it in the fd cleanup method. It was the design intention of doneConnecting() to cleanup the FD state and be the single exit point for the Job. swanSong() will call doneConnecting() with error flag if it is reached and doneConnecting() has not been run yet to do that cleanup. The timeout() and connect() handlers should do whatever cleanup they need to override what doneConnecting() cleanup would do. ie timeout() clearing the calls_.timeout_ instead of letting it be canceled with a bogus message. +ptr = static_cast(F->write_data); +delete ptr; This is not going to work well. F->write_data now points to deleted memory.
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 18/01/2013 10:53 a.m., Rainer Weikusat wrote: Alex Rousskov writes: On 01/17/2013 10:56 AM, Rainer Weikusat wrote: Amos, do you remember what "partially open FD" in the following comment meant, exactly? Why cannot we just call comm_close() to cleanup? // it never reached fully open, so cleanup the FD handlers // Note that comm_close() sequence does not happen for partially open FD In your latest patch, please move fd cleanup from timeout() and from doneConnecting() into a dedicated method and call that method from swanSong() if temporaryFd_ is still set there. Do not call it from doneConnecting() or anywhere else. Remember that a job may end for reasons other than some timeout or I/O. swanSong() is the cleanup method. Please do not forget to set calls.earlyAbort to NULL after canceling it in the fd cleanup method. What precisely constitutes 'fd_cleanup'? The doneConnecting code will set calls_.early_abort to NULL and the leading comment states /** * Connection attempt are completed. One way or the other. * Pass the results back to the external handler. * NP: on errors the earlyAbort call should be cancelled first with a reason. */ This means that it won't be possible to cancel the call afterwards (which may not be necessary) but documenting the 'pre calling procedure' of some routine as "do such-and-such a thing before calling this" and then not doing it when calling the routine seems confusing to me. This is a case where the code was changed but the documentation left without updating. s/should be cancelled/ should have been cancelled/ +ptr = static_cast(F->write_data); +delete ptr; This is not going to work well. F->write_data now points to deleted memory. If you absolutely have to do this, please set it to NULL after delete. Insofar my understanding goes, the Comm::SetSelect(temporaryFd_, COMM_SELECT_WRITE, NULL, NULL, 0); in doneConnecting will do that (and - for good measure - the fd_close called from doneConnecting will clear the write pointers again). Yes, but the SetSelect needs to be kept paired close to the fiddling with write_data. This is a hack, polishing it now is not a good idea. The purpose of it is to keep the write handler state consistent. Be conservative with the hack/workaround and don't depend on all other code behaviour remaining the same indefinitely. In fact, if the workaround needs to be done at more than one place of the code it needs to be in a separate private method and documented as a hack in the methods docs. Amos
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 18/01/2013 2:51 p.m., Alex Rousskov wrote: On 01/17/2013 02:53 PM, Rainer Weikusat wrote: Alex Rousskov writes: On 01/17/2013 10:56 AM, Rainer Weikusat wrote: Then, the ::connect method (or some code running on behalf of that) would close temporaryFd_ before scheduling a 'delayed connect retry' which would open a new socket. A ::timeout running then won't need to free write_data and won't have to cancel the early_abort call. Agreed. If the write handler was scheduled by the time timout runs, this happened because 'something definite' is known about the state of the connection attempt: Either, it suceeded or the OS returned an error. Throwing this 'real information' away in order to save a line of code seems not right to me even if it is a fringe case of a fringe case. As I said, one would not be just saving lines of code. One would be most likely introducing bugs (that is exactly what happened in the past with this code). We have at most two "real information" pieces: a timeout and I/O status. The first to get to ConnOpener wins. We have the problem that we do not know whether the future scheduled ::connect was scheduled with a success (usually is) or a IN_PROGRESS which would re-start another in-progress event cycle. The Job won't know that datum until that scheduled Call is run. This is the exact edge case which the existing trunk code was written to handle and did not work because the OS connect() handling has proven to be unreliable. Where timeout needs to abort, even if ::connect() has succeeded but taken 1ms too long (with a scheduled Call). Still a Job exit on timeout error in the new design you are writing. Amos
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 18/01/2013 5:37 p.m., Alex Rousskov wrote: On 01/17/2013 06:35 PM, Amos Jeffries wrote: Amos, do you remember what "partially open FD" in the following comment meant, exactly? Why cannot we just call comm_close() to cleanup? It means we have issued socket() and connect() OS calls. fd_table[] state probably exists. But we are still waiting on either TCP response, or the In-Progress event to run and the FD to be confirmed as open. This is signalled by (temporaryFd_ >= 0). Why cannot we just call comm_close() to cleanup Comm state? Because comm_openex() and comm_close() are not "symmetrical" in what they do. comm_close() involves a lot of handlers cleanup and close code for state objects which simply do not exist yet. The FD has only had socket() and connect() done on it and a few specific things done to the fd_table. One of which is registering the calls_.earlyAbort_ handler as what gets run on comm_close() in case the Squid shutdown/restart sequence or TCP close happens during this Jobs operations. If we use comm_close() most of the required cleanup will never happen. Amos
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 18/01/2013 6:47 p.m., Alex Rousskov wrote: On 01/17/2013 10:28 PM, Amos Jeffries wrote: On 18/01/2013 5:37 p.m., Alex Rousskov wrote: On 01/17/2013 06:35 PM, Amos Jeffries wrote: Amos, do you remember what "partially open FD" in the following comment meant, exactly? Why cannot we just call comm_close() to cleanup? It means we have issued socket() and connect() OS calls. fd_table[] state probably exists. But we are still waiting on either TCP response, or the In-Progress event to run and the FD to be confirmed as open. This is signalled by (temporaryFd_ >= 0). Why cannot we just call comm_close() to cleanup Comm state? Because comm_openex() and comm_close() are not "symmetrical" in what they do. comm_close() involves a lot of handlers cleanup and close code for state objects which simply do not exist yet. Why non-existing handlers and objects are a problem? We have a write handler and a close handler registered with Comm, right? Why not call comm_close() to clean them up (from Comm point of view) and close the descriptor? We do not need those handlers to be actually called (we may even cancel them before calling comm_close). We just need to clear Comm state and close the descriptor. Will comm_close() break if we call it instead of reaching into Comm guts and manipulating its tables? If yes, what is Comm missing? Perhaps we can give that missing piece to Comm so that we can comm_close()? * Notice that until ConnOpener has finished the FD is not open, as in "if(isOpen(fd)) return;" produces false, so comm_close() will exit before it gets to any cleanup. Removing that check will result in trouble processing some FD types which currently use comm_close(). * The registered close handler is earlyAbort which flags the FD as error instead of timeout or success. * fd_close(temporaryFd_), which we use already, cleans up all the non-ConnOpener state which is associated with the FD. It is simpler just to prune out the two bits of ConnOpener state in ConnOpener itself rather than rely on the generic close sequence, which has to do a lot of checking for irrelevant things. The whole fiddling with write handler complexity is a hack internal to ConOpener anyway. Otherwise it could just be SetSelect(..., NULL,...) and be done. The FD has only had socket() and connect() done on it and a few specific things done to the fd_table. One of which is registering the calls_.earlyAbort_ handler as what gets run on comm_close() in case the Squid shutdown/restart sequence or TCP close happens during this Jobs operations. If we use comm_close() most of the required cleanup will never happen. Please note that I am _not_ suggesting to call comm_close to clean up our job state (like a lot of Squid code does) even though I believe it would be possible if doneAll() is adjusted accordingly. I am just trying to find a way to call comm_close() to clean up Comm state without violating Comm APIs. We will clean the job's state in addition to calling comm_close(). ConnOpener lives in the limbo area between fd_open()/fd_close() which sets up the TCP state on an FD and comm_open()/comm_close() which setup and tear down the mountain of comm transaction I/O state on an FD. From any point after ConnOpener move temporaryFd_ into conn->fd (finished with success) comm_close() is the suitable way to cleanup. On the other error/timeout exit points from ConnOpener fd_close()+unwinding is the more appropriate way to cleanup [because Comm failed to setup the FD] and nothing outside of ConnOpener is ever informed about an FD# which enforces that condition. You may indeed be able to re-write comm_close() such that it will work on any FD, including one which failed to reach success in ConnOpener. But I think that is a much more difficult/larger project outside the scope of the priority bug-fixes Rainer is working on here. Amos
Re: [PATCH] Propagate pinned connection persistency and closures to the client.
On 18/01/2013 2:34 a.m., Tsantilas Christos wrote: Hi all, this patch removes the "repinning" code from squid. A related discussion in squid-dev mailing list is here: http://www.mail-archive.com/squid-dev@squid-cache.org/msg18997.html Detailed description of the patch follows: Propagate pinned connection persistency and closures to the client. Squid was trying hard to forward a request after pinned connection failures because some of those failures were benign pconn races. That meant re-pinning failed connections. After a few iterations to correctly handle non-idempotent requests, the code appeared to work, but the final design, with all the added complexity and related dangers was deemed inferior to the approach we use now. Squid now simply propagates connection closures (including pconn races) to the client. It is now the client responsibility not to send non-idempotent requests on idle persistent connections and to recover from pconn races. Squid also propagates HTTP connection persistency indicators from client to server and back, to make client job feasible. Squid will send Connection:close and will close the client connection if the pinned server says so, even if Squid could still maintain a persistent connection with the client. These changes are not mean to affect regular (not pinned) transactions. In access.log, one can detect requests that were not responded to (due to race conditions on pinned connections) by searching for ERR_ZERO_SIZE_OBJECT %err_code with TCP_MISS/000 status and zero response bytes. This is a Measurement Factory project. Thank you. All I can see standing out are a few cosmetic polishes. In src/client_side_reply.cc: * "not sending to a closing on pinned zero reply " does not make any sense. Did you mean "not sending more data after a pinned zero reply "? although even that is a bit unclear. Since this is public log text I think we better clarify it a lot before this patch goes in. * please remove HERE from the new/changed debugs lines in selectPeerForIntercepted(). Amos
Re: [PATCH] Avoid abandoned client connections after url_rewriter redirects CONNECT
On 17/01/2013 6:47 a.m., Alex Rousskov wrote: Hello, clientProcessRequest() assumes that a CONNECT request is always tunneled and sets flags.readMore to false. However, if url_rewriter redirects the CONNECTing user, Squid responds with a redirect message and does not tunnel. In that case, we do want to read more. Otherwise, keepaliveNextRequest() will declare the connection abandoned and the connection descriptor will "leak" until the connection lifetime expires, even if the client disconnects immediately after receiving the redirect response. The fix delays setting flags.readMore to false until we are about to call tunnelStart(). The effect on CONNECT authentication (another case where CONNECT is not tunneled) is untested, but I hope that code continues to work because it should be OK with reading more requests on the [being] authenticated connection. These changes may also fix other similar not-tunneled CONNECT cases. They are not related to SslBump. One of the attached patches is for trunk. The other one is for v3.2. I did not check whether the problem exists in v3.1. HTH, Alex. +1. It looks okay, although I would like confirmation from somebody using NTLM authentication that things still work afterwards. Amos
Re: [PATCH] Avoid abandoned client connections after url_rewriter redirects CONNECT
On 19/01/2013 11:53 a.m., Alex Rousskov wrote: On 01/18/2013 05:32 AM, Amos Jeffries wrote: On 17/01/2013 6:47 a.m., Alex Rousskov wrote: Hello, clientProcessRequest() assumes that a CONNECT request is always tunneled and sets flags.readMore to false. However, if url_rewriter redirects the CONNECTing user, Squid responds with a redirect message and does not tunnel. In that case, we do want to read more. Otherwise, keepaliveNextRequest() will declare the connection abandoned and the connection descriptor will "leak" until the connection lifetime expires, even if the client disconnects immediately after receiving the redirect response. The fix delays setting flags.readMore to false until we are about to call tunnelStart(). The effect on CONNECT authentication (another case where CONNECT is not tunneled) is untested, but I hope that code continues to work because it should be OK with reading more requests on the [being] authenticated connection. These changes may also fix other similar not-tunneled CONNECT cases. They are not related to SslBump. One of the attached patches is for trunk. The other one is for v3.2. I did not check whether the problem exists in v3.1. HTH, Alex. +1. It looks okay, although I would like confirmation from somebody using NTLM authentication that things still work afterwards. My patches broke SslBump -- Squid started reading from the client connection before switching to SSL :-(. The attached patches were tested with SslBump (and without). They are more conservative/less invasive: I now leave readMore set to false for CONNECT until it is clear that Squid is not going to tunnel or bump. This should break fewer cases. I think the correct way will be to take your first patch, but shuffle the use of stopReading() above tunnelStart() to above both tunnelStart() and sslBumpStart(), then add a readMore=true when sslBump setup is completed. If this works it confirms a suspicion of mine that sslBumpStart() should be moved inside tunnelStart() and that function should be the place to select between handling CONNECT as ssl-bump / Upgrade: / or blind tunnel. Amos
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 19/01/2013 12:19 p.m., Rainer Weikusat wrote: Amos Jeffries writes: On 01/17/2013 02:53 PM, Rainer Weikusat wrote: Alex Rousskov writes: On 01/17/2013 10:56 AM, Rainer Weikusat wrote: Then, the ::connect method (or some code running on behalf of that) would close temporaryFd_ before scheduling a 'delayed connect retry' which would open a new socket. A ::timeout running then won't need to free write_data and won't have to cancel the early_abort call. Agreed. If the write handler was scheduled by the time timout runs, this happened because 'something definite' is known about the state of the connection attempt: Either, it suceeded or the OS returned an error. Throwing this 'real information' away in order to save a line of code seems not right to me even if it is a fringe case of a fringe case. As I said, one would not be just saving lines of code. One would be most likely introducing bugs (that is exactly what happened in the past with this code). We have at most two "real information" pieces: a timeout and I/O status. The first to get to ConnOpener wins. We have the problem that we do not know whether the future scheduled ::connect was scheduled with a success (usually is) or a IN_PROGRESS which would re-start another in-progress event cycle. The Job won't know that datum until that scheduled Call is run. Presently, that's not much of a problem because the retry code is broken since it doesn't really retry the connect but just re-schedules the ::connect method call which will call comm_connect_addr with an unchanged state which should (I haven't tested this) either return the previous error or (more likely) return another 'false positive' 'connection succeeded' result which should ultimatively trigger the same assert I wrote about in my first mail to this list. After fixing this, the ::connect code could look at the calls_.timeout_ member to determine if it is still supposed to retry or if a timeout happened in between and retries should stop. If the timeout already happened then doneConnecting() has already stopped the Job and the connect() Call will not be run as Alex pointed out. calls_.* are local pointers held by ConnOpener for the sole purpose of calling cancel() when necessary. They are *not* unset when the event system of Squid schedules the Call. There is a separate pointer held in the FD system (fd_table[].*Handler) which is cleared when the Call is scheduled by event system or unset via Comm::SetSelect. This Comm::ConnOpener layer is not exactly a good place to be playing with those directly (the write_data is a problem hack and needs to die ASAP, but not yet). NP: We have already agreed to throw away the later arriving or bogus 'connection succeeded' information if timeout() wins the Call queue race and uses doneConnecting(). Amos
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 19/01/2013 12:32 p.m., Rainer Weikusat wrote: Alex Rousskov writes: On 01/17/2013 02:53 PM, Rainer Weikusat wrote: [...] If the write handler was scheduled by the time timout runs, this happened because 'something definite' is known about the state of the connection attempt: Either, it suceeded or the OS returned an error. Throwing this 'real information' away in order to save a line of code seems not right to me even if it is a fringe case of a fringe case. As I said, one would not be just saving lines of code. One would be most likely introducing bugs (that is exactly what happened in the past with this code). We have at most two "real information" pieces: a timeout and I/O status. The first to get to ConnOpener wins. Since F->write_handler will be cleared prior to the write handler being scheduled, the ::timeout code could exploit this property to determine reliably if an I/O status is available. The question would be 'is this useful enough to warrant otherwise needlessly digging around in the guts of another module'? This is the debate I lost with Alex. We are talking sub-millisecond race resolution on something which has already passed >1 minute of wait time at the client end. There is a high chance that we have also lost the client disconnection race some whole seconds ago, or that processing this timeout() is holding up an I/O select() Call which will change that write handlers status. I still think it would be worth checking if it were proven that calling connect() again would be reliable. But the benefit of doing so in the current code is outweighed by the complexity needed for reliability. A 'nicer' solution would be to add an interface to comm which could look like this: void *comm_kill_read_handler_if(int fd); void *comm_kill_write_handler_if(int fd); which would do the necessary fd_table manipulations to kill a read or write handler and return the read_data/ write_data pointer if a handler was actually registered. Considering 'This is a hack, polishing it now is not a good idea.' (<50f8d7d3.3010...@treenet.co.nz>), I think you're right and not checking for this is the better choice. Yes, we an come back and look at this after the SetSelect() code is upgraded to AsyncCall awareness. Amos
Squid 3.3 countdown
As I mentioned in the last release announcement for 3.3. This month I am ticking off the requirements for 3.3 to go stable. As it stands we have four bugs which appear to be major enough to block that release: * http://bugs.squid-cache.org/show_bug.cgi?id=3740 - "ssl_bump none ..." apparently not working. You on this one Alex? * http://bugs.squid-cache.org/show_bug.cgi?id=3743 - ssl_crtd not sending fatal startup errors to stderr Any taker? Christos? * http://bugs.squid-cache.org/show_bug.cgi?id=3735 - crash on raw-IPv6 requests. + Waiting on feedback. * http://bugs.squid-cache.org/show_bug.cgi?id=3350 - ACL result type screwing up cache_peer ACLs. + I *think* this is resolved now. Especialy since I am using a Squid version with peers and ACLs where this is supposed to be broken. Then there are the older versions bugs in major and critical we should be fixing as well. Which is now out at 62, an annoying 16 new major bugs in 3.2 since it went stable. Amos
Re: [PATCH] Move timeout from ConnOpener::connect to ConnOpener::timeout
On 21/01/2013 9:21 a.m., Rainer Weikusat wrote: The attached patch removes the timeout checking code from ConnOpener::connect and changes ConnOpener::timeout to actually force the connection attempt to be terminated with 'a timeout error' instead of invoking the connect method. The main motivation for this is that a timeout occuring for a 'server first' SSL bumped connection attempt would otherwise trigger an assert in forward.cc because of the lack of either 'an I/O result' or an error state after the comm_connect_addr routine invoked from connect after the connect call from timeout had erroneously signalled that the connection was successfully established to its caller. +1 with tweaks: the definitions of *F and *prt need to be inside their relevant if() scopes. But otherwise this looks fine for the timeout move. Amos
Bug 3111:
Hi Alex, could you clarify the situation you mention in http://bugs.squid-cache.org/show_bug.cgi?id=3111 please? You said there that ConnOpener leaves events scheduled on successful exit. But, I was of the understanding that we should NOT explicitly clear and cancel any Calls in ConnOpener()::connected() due to duplication with the swanSong() actions run after that successful connect Call ended? Amos
Re: [PATCH] Consumption of POST body
On 21/01/2013 11:54 p.m., Steve Hill wrote: On 18.01.13 18:37, Alex Rousskov wrote: However, this brings us back to trunk r11920 and bug 3420. Ok, reading that bug it seems I shouldn't be calling enableAutoConsumption() directly - this needs to be handled through expectNoConsumption()? I rather suspected this was going to be the case to be honest - calling enableAutoConsumption() directly seemed messy and probably wrong. Indeed, having two methods with approximately the same purpose and a hidden critical distinction is asking for trouble. A lot of existing expectNoForwarding-path code comments describe the situation that matches yours. Moreover, as far as I can see, we may call the old expectNoForwarding() first and then call your forwardToBitbucket() later, all for the same error! I'm not sure under what circumstances we'd ever want the current expectNoForwarding() behaviour (i.e. closing the socket immediately). It seems to me that whatever happens, we should be allowing the client's request to conclude (possibly with some timeout to prevent the client from tieing up the connection for forever). As far as I can tell, we ideally have the following options: 1. Return a response with Connection: keep-alive, wait until we've received the whole request from the client and then start reading the next request. 2. Return a response with Connection: close, wait until we've received the whole request from the client and then close the socket. This (2) opens the door for the "Slow-Loris" style of attacks. If we are sure there is nothing to follow, AND the server-side is not using the data we should close as soon as possible. IIRC this early closure is the case we have comm_lingering_close() for, to ensure both FIN packets and any pending data are handled gracefully. As an alternative if we don't care about the client receiving any data/error page from our side we can use comm_reset_close() to send a TCP RST packet down the line. So I'm not sure there's ever any requirement to ditch the connection before we've got the whole request - obviously in the case of a completely fatal error it would be nice to not have to deal with the overhead of a large POST that is just going to end up in the bitbucket, but I'm not sure we actually have a choice since closing the socket early may not be handled well by the client. 1. Authentication: Must read next request on the same connection so must have readNextRequest set (if not, this is probably a bug). Have to live with the possibility of the client sending forever. I suppose whether a client can send forever is dependent on the encoding its using. If the client has set a Content-Length header then it can only send that much content. If the client is using chunked encoding then no upper limit on content length is required (I think?) so in this case maybe squid needs to impose its own limit and chop the connection if its exceeded. In either case, a client sitting idle for a long time should probably be dropped anyway. Yes. read_timeout during a transaction, client_idle_pconn_timeout when between requests. 2. Adaptation errors: The code may have been written to abort the client connection, but it also sets readMore or readNextRequest. This sounds inconsistent. We should review this, especially since this is the only place where we call expectNoForwarding() today! Just to be clear, in what circumstances does the adaption code need to abort the client connection? The only one I can think of off the top of my head is where something breaks half way through handling the response body (i.e. we haven't actually delivered an error to the client - we've started delivering a real response but can't complete it). As an aside, the adaption errors probably need a bit of review anyway, I especially like the "Communication with the ICAP server produced the following error: No error" error messages. :) (Can't recall the exact wording, but its along those lines). * Use expectNoForwarding() instead of adding forwardToBitbucket(). * In ConnStateData::noteBodyConsumerAborted() do not stop reading if flags.readMore is set. Just keep auto-consuming. This may prevent authenticating connections closures even though expectNoForwarding() is used. Needs to be tested: Will we correctly stop and switch to the new request when the body finally ends? I shall have a look at these today (hopefully :) and see if I can engineer a better patch. * Discuss and possibly remove readNextRequest setting from ClientHttpRequest::handleAdaptationFailure(), to remove inconsistency discussed in #2 above. I'll have a look at this, although I'm quite unfamiliar with that bit of code so may not be able to understand it well enough. I *think* that might be a good idea. The way HTTP/2 is progressing we are going to be multiplexing requests on each connection very shortly. At the framing level each request will be isolated from others rat
Re: [PATCH] Consumption of POST body
On 22/01/2013 3:04 a.m., Steve Hill wrote: On 18.01.13 18:37, Alex Rousskov wrote: Further to this... * Use expectNoForwarding() instead of adding forwardToBitbucket(). * In ConnStateData::noteBodyConsumerAborted() do not stop reading if flags.readMore is set. Just keep auto-consuming. This may prevent authenticating connections closures even though expectNoForwarding() is used. Needs to be tested: Will we correctly stop and switch to the new request when the body finally ends? I have changed to using expectNoForwarding() and removed the stopReceiving() call from noteBodyConsumerAborted() and this appears to work correctly: 1. Making a request which results in an error, with "Connection: close" produces a response to the client, and then once the request body has been fully received, Squid closes the connection. Does this work with a 70,000 clients all doing 100 byte request bodies sent 1 byte per minute (header sent normally)? The DoS attack PoC expectation is that the proxy becomes unresponsive for about an hour while the whole TCP available socket range has ESTALISHED connections. The normal Squid operation expects that no TCP sockets held in ESTABLISHED state past the first second, and resumes full availability within 15 minutes. Although some requests may fail if the sockets stay in TIME_WAIT for long. If possible the TIME_WAIT is avoided and full availability is never lost. 2. Making a request which results in an error, with "Connection: keep-alive" produces a response to the client, and then once the request body has been fully received Squid reads the next request. 3. Making a request which does *NOT* result in an error, with "Connection: keep-alive" works as expected and then waits for the next request. 4. Making a request which does *NOT* result in an error, with "Connection: close" works as expected and then closes the connection. 5. Making a request which results in an ICAP error (I shut down a non-bypassable ICAP service), with "Connection: keep-alive" produces a 500 error response and then waits for the next request. 6. Making a request which results in an ICAP error, with "Connection: close" produces a 500 error response and then Squid closes the connection. I don't really understand the purpose of the readMore flag, can you explain it? It's a hack due to some internals of Squid needing direct read-access to the client socket to perform their actions. SSL handshake and CONNECT tunnel body handling being the big remaining ones these days. It signals the ConnStateData object which is normally responsible for the I/O read handler that it is *not* to process any incoming data on the socket until the flags is set back to true. It is/was very likely also being abused to signal that the error cases have occured and the input is to be dropped by TCP shutdown. * Discuss and possibly remove readNextRequest setting from ClientHttpRequest::handleAdaptationFailure(), to remove inconsistency discussed in #2 above. As mentioned above, I don't understand what readMore is actually for. However, as I mentioned in my previous email, the only time I can see us wanting to blindly close the connection for adaptation failures is when we've already started sending a response to the client and the adaptation service blows up in the middle of the response body. It doesn't _look_ like handleAdaptationFailure() handles that event though, unless I'm mistaken? * Discuss whether we are OK with clients tying up Squid after an [authentication] error. Perhaps we need another timeout there, perhaps there are already mechanisms to address that, or perhaps we do not really care. There are 4 possible ways I can see of a client tieing up Squid: 1. The client connects but doesn't send anything. 2. The client connects, makes a keep-alive request that results in an error, then doesn't send any new request. 3. The client makes a request that generates an error, but doesn't send the complete request body. 4. The client makes a request that generates an error, uses chunked encoding and sends an infinitely long body. (1) and (2) are basically the same thing. I've tested this and Squid times out the connections and closes them after a few minutes. (3) similarly gets timed out after a while. I haven't tested (4), but I have no reason to believe there's anything that currently stops it from happening. I'm not sure whether or not we should do something to prevent it - that's one for discussion. If the data is being bropped on arrival by Squid we should be watching for these cases and aborting client connections which go on too long. I suspect the read_timout timer should be kept ticking on these reads and not reset to 0. That would cap the abuse time at 15 minutes per client rather than infinite. Amos
Re: [PATCH] Consumption of POST body
On 22/01/2013 9:15 a.m., Tsantilas Christos wrote: On 01/18/2013 08:37 PM, Alex Rousskov wrote: On 01/18/2013 08:14 AM, Steve Hill wrote: The attached patch enables autoconsumption of the BodyPipe if access isn't allowed in order to discard the request body rather than letting it fill the buffers. Please note that the patch enables auto-consumption for all callout errors, not just access denials. Also, the trunk code is even more complex in this area. I believe your patch, when carefully ported to trunk, may enable auto-consumption for all callout errors *that have readNextRequest set*. That makes sense to me: if we are going to read the next request, we should consume the current one without closing the client connection so we should not call expectNoForwarding() that may have that side effect. The overall intent sounds correct to me. Even if we do not want to keep the connection open after responding with an error, it may be impossible to deliver the error message to a half-broken client that does not read while writing to us. However, this brings us back to trunk r11920 and bug 3420. While that fix was triggered by an assertion, it is clear from the code comments and the commit message that we were worried about a client tying Squid client connection "forever" (with or without sending data). Your changes will re-enable that behavior in some cases, I guess. More on that below. I'm not entirely sure this is the best fix - calling expectNoForwarding() would seem to be neater, but this also causes the client-side socket to close, which would break authentication mechanisms that require the connection to be kept alive. Hopefully someone with a better understanding of the Squid internals can review the patch and tell me if it is the right approach? Thank you very much for treading carefully and disclosing potential problems! Indeed, having two methods with approximately the same purpose and a hidden critical distinction is asking for trouble. A lot of existing expectNoForwarding-path code comments describe the situation that matches yours. Moreover, as far as I can see, we may call the old expectNoForwarding() first and then call your forwardToBitbucket() later, all for the same error! As far as I can tell, we have several cases to take care of (or ignore) here: 1. Authentication: Must read next request on the same connection so must have readNextRequest set (if not, this is probably a bug). Have to live with the possibility of the client sending forever. In this case we may have problem only for NTLM or similar authentication. But in this case imagine that the HTTP client post a huge file and Squid respond with an "401 Unauthorised". Is it normal to read all of the file in this case? "Maybe.". Some clients work okay some don't. Like IE6 requires it and Sharepoint, Exchange, etc do not work if its sent. We are going to read all of the file in the next authenticated request too. So for one request we have to read ALL the posted file twice. It is not normal. I believe that the best is to send the error page and abort. The browser should handle early replies to support such cases... Also the curl version I have supports early replies, I used custom perl client to reproduce the problem... HTTP allows us to close a connection at any time. For NTLM handshakes we can (configurably) close the connection after the first 401 message, but not the second, and are again on the third request - which should have succeeded. Amos
Re: [PATCH] Move timeout from ConnOpener::connect to ConnOpener::timeout
On 22/01/2013 5:53 a.m., Rainer Weikusat wrote: Amos Jeffries writes: On 21/01/2013 9:21 a.m., Rainer Weikusat wrote: The attached patch removes the timeout checking code from ConnOpener::connect and [...] the definitions of *F and *prt need to be inside their relevant if() scopes. +1. Looks good now. Amos
[RFC] default HTCP
With HTTP/1.1 functionality Variants are much more common and the ICP failure rate is growing quite high. I propose altering the default behaviour of peering to using HTCP queries by default. This would entail adding "htcp=off" and "icp" options to cache_peer and altering the build defaults such that HTCP was always available. The "no-query" option is vague enough that it can be left with its existing behaviour modified to disable the default queries. Amos
Re: [PATCH] Unpack bools in PortCfg
On 23/01/2013 11:52 p.m., Kinkie wrote: Hi all, the attached patch unpacks the unsigned int:1 declarations for flags in PortCfg and turns them to unpacked bools. -- /kinkie This clashes with a patch I have nearly ready separating the flags into groups and renaming for clarty. Can we merge the two? Amos
Re: [PATCH] Coverity issue 740457
On 24/01/2013 4:30 a.m., Kinkie wrote: This seems to be droppign the small mkstemp optimization makign use of its FD return result as the already open file. I think we should be calling umask() before before the whole #if segment and reset it on each of "return" as well as on successful completion. Hi, v2 of the patch, which does as you suggest (and removes a few useless empty lines). -- /kinkie That looks much better. * the string created by tempnam() apparently needs to be xfree()'d when done. * please add a comment in the #else condition noting the tempnam() is officially obsolete from POSIX-2008.1. (in a few more years we should drop the #else code entirely.) * please also add a note that tmpfile() is not an option since we desire the /tmp file to stick around after process exit for administrators use. As a followup change we need to consider that this whole process leaves a /tmp file sitting around from every crash, which could over time build up. I'm in two minds about how desirable this behaviour is. + we could check for the files on startup and do something when Squid is re-started after a crash. + administrator can manually find the file even if the mail attempt failed (known to happen). - the files build up over time and could be a big waste of disk space. Amos
Re: [PATCH] Unpack bools in ACLUserData
On 24/01/2013 5:22 a.m., Kinkie wrote: Hi, this patch converts the unsinged integers in ACLUserData to unpacked bools. -- /kinkie Are we doing this to reduce the memory footprint? or only to simplify the code? This step goes some way towards memory savings by halving the memory footprint. To reduce the memory footprint by another half again we should be using bool:1. In micro-benchmarks I ran 100,000,000 X.x=1 assignments; uint->bool saves ~20 to 100 ms, bool->bool:1 saves ~5 to -90 ms more) Amos
Re: [PATCH] bool-ify CachePeer
On 24/01/2013 7:20 a.m., Kinkie wrote: Hi all, the attached patch turns the unsigned int:1 flags in CachePeer to bools. -- /kinkie Please retain the :1 bitmasking. My microbench is showing a consistent ~50ms speed gain on bitmasks over full bool, particularly when there are multiple bools in the structure. We also get some useful object size gains. Amos
Re: [PATCH] Propagate pinned connection persistency and closures to the client.
On 24/01/2013 10:27 a.m., Tsantilas Christos wrote: This is a new version of the patch which handles Amos comments. If there is not any objection I will commit this patch to trunk... +1. Amos
Re: [PATCH] Avoid abandoned client connections after url_rewriter redirects CONNECT
On 24/01/2013 10:58 a.m., Alex Rousskov wrote: On 01/18/2013 08:43 PM, Amos Jeffries wrote: On 19/01/2013 11:53 a.m., Alex Rousskov wrote: On 01/18/2013 05:32 AM, Amos Jeffries wrote: On 17/01/2013 6:47 a.m., Alex Rousskov wrote: Hello, clientProcessRequest() assumes that a CONNECT request is always tunneled and sets flags.readMore to false. However, if url_rewriter redirects the CONNECTing user, Squid responds with a redirect message and does not tunnel. In that case, we do want to read more. Otherwise, keepaliveNextRequest() will declare the connection abandoned and the connection descriptor will "leak" until the connection lifetime expires, even if the client disconnects immediately after receiving the redirect response. The fix delays setting flags.readMore to false until we are about to call tunnelStart(). The effect on CONNECT authentication (another case where CONNECT is not tunneled) is untested, but I hope that code continues to work because it should be OK with reading more requests on the [being] authenticated connection. These changes may also fix other similar not-tunneled CONNECT cases. They are not related to SslBump. One of the attached patches is for trunk. The other one is for v3.2. I did not check whether the problem exists in v3.1. HTH, Alex. +1. It looks okay, although I would like confirmation from somebody using NTLM authentication that things still work afterwards. My patches broke SslBump -- Squid started reading from the client connection before switching to SSL :-(. The attached patches were tested with SslBump (and without). They are more conservative/less invasive: I now leave readMore set to false for CONNECT until it is clear that Squid is not going to tunnel or bump. This should break fewer cases. I think the correct way will be to take your first patch, but shuffle the use of stopReading() above tunnelStart() to above both tunnelStart() and sslBumpStart(), then add a readMore=true when sslBump setup is completed. I do not think that is a good idea for two reasons: 1) As you know, both tunnelStart() and sslBumpStart() are called from processRequest(). A lot of things can happen _before_ we get to that method if we allow readMore to remain true while we are getting there! Some of those things will probably lead to bugs. For example, Squid may read [encrypted] data from the client socket and try to interpret that as the beginning of the next request (or the body of the current?). This is what I may have seen in the logs from take1 tests (I did not investigate, but it sounds quite possible based on the code). The key here is that ClientSocketContext::keepaliveNextRequest() and/or clientAfterReadingRequests() may start reading and interpreting again if readMore is true while doCallouts() for the previous request are still "waiting" for some async call to return. Granted, it may be possible that the same kind of bad things already happen without my patches, but I do not want to add more of them (because I would end up being responsible for fixing all of them as it would appear that I broke something that worked). 2) The problem with take1 of the patch was not just that we started reading from the client while SslBump was being setup. We also started reading from the client when responding with an error (including authentication?) or a redirect. I am seriously worried that if we do not disable client reading, we will hit assertions elsewhere on that path because Squid client-side is not very good at handling a new request while the old one is still being processed, especially in corner/error cases like the ones I just mentioned. I think it is much safer to remain conservative here and disable reading until it is known to be safe. Did the above arguments convince you that take2 version of the patch is better than what you are proposing? Yes that and some discussion I have been having recently about HTTP/1.x framing convince me you are on the right track with patch #2 for now. We will need to revisit this logic when adding HTTP/2 support. The framing there is a LOT better, wven if it ends up being the session-centric SPDY frame design. If this works it confirms a suspicion of mine that sslBumpStart() should be moved inside tunnelStart() and that function should be the place to select between handling CONNECT as ssl-bump / Upgrade: / or blind tunnel. It would be wrong to start a non-tunneling path in a function called tunnelStart() IMO. Currently, ClientHttpRequest::processRequest() is where various "start" methods are called and where the decision to tunnel or bump is made for CONNECT requests. You are overlooking that httpStart() is where ftp:// and Gopher:// and internal:// and ICY protocol are handled. Makign it clear these function names are all out of true with what they do. Also, ssl-bump is a special-case handling of tunnel data. So I see no conceptual clash with tunnelStart() deciding
Re: [PATCH] Mimic Key Usage and Basic Constraints
On 24/01/2013 11:13 a.m., Tsantilas Christos wrote: There are cases where the generated certificates do not mimic enough properties and secure connection with the client fails. For example, Squid does not mimic Key Usage extensions. Clients using GnuTLS (or similar libraries that validate server certificate using those extensions) fail to secure the connection with Squid. This patch add mimicking for the following extensions, which are considered as safe to mimic: * X509v3 Key Usage * X509v3 Extended Key Usage, * X509v3 Basic Constraints CA. We would be happy to add more "safe to mimic" extensions if users request (and vouch for) them. This is a Measurement Factory project Regards, Christos +1. So long as they are safe. The code looks okay anyway. Amos
Re: [PATCH] schedule connect timeouts via eventAdd
On 24/01/2013 1:50 p.m., Alex Rousskov wrote: On 01/23/2013 04:39 PM, Rainer Weikusat wrote: Rainer Weikusat writes: [...] The downside of this approach is that this means adding future connect timeouts will become proportionally more expensive as more and more connections are being established This is an understatement. Based on a somewhat simplified model of the event list (initially empty, only connect timeouts), adding n timeouts in a row would have a cost cost 2 * (n - 1) + 1 (assuming 'insertion cost/ empy list' 1, same for removal cost) when adding and removing the events but would be (n * (n + 1)) / 2 for the other case (sum of 1 + 2 + ... + n). Side note: I am very glad you noticed that problem with event-based timeouts. This tells me somebody is thinking about the code beyond the itch to squash a known bug. :-) To simplify your analysis and make it more meaningful, please consider a typical steady state, where you have R new connections per second, t second connection establishment delay, T second timeout, and negligible number of actual timeouts. In that state, one event-based approach gives you R*t timeouts and the second event-based approach gives you R*T timeouts registered with Squid at any time. What are the cost of handling one event-based timeout in the first (add and forget) and second (add and remove) event-based approaches? Since we start search from the oldest event and all timeouts will go to the very end of the list, I think the costs are: add-and-forget: R*T add-and-remove: 2*R*t (*) The 2 multiplier for add-and-remove is kind of the worst case -- in general, the event we want to cancel will be in the beginning of the queue, not the end of it! I think there is a ^ calculation missing in the add-and-forget formula. Because the list growth is exponential the add() cost rises exponentially for each R+1. If the above model is correct, the best choice should be clear by now because a typical 2*t is a lot less than a typical T, but let's try R = 1000 new connections per second, T = 60 seconds (default), and t = 0.1 second (conservative!). add-and-forget: 1000 * 60 = 60'000 operations add-and-remove: 2 * 1000 * 0.1 =200 operations Still "200" or even "100" (*) is a significant cost for this more-or-less realistic example. We can reduce that cost if we add an "add by searching from the end of the queue" eventAdd() optimization (either automated or explicit). In that case, the costs will become: add-and-forget-o: 1 add-and-remove-o: R*t Or we could go back to fd-based timeouts, but we would need to be extra careful with maintaining them using the same raw Comm manipulations that have screwed us in the past. That would be especially difficult across changing temporary FDs... Cost summary: CPU RAM current fd-based: 1 R*t add-and-forget-o: 1 R*T add-and-remove-o:R*t R*t Are my estimates correct? Is storing 60'000 events instead of 100 acceptable? I kind of doubt it is... :-( I agree. Particularly since there are ~10 other _types_ of event sharing the queue. With varying length of timeout on each type. This means that add/remove operations they are doing on their own (currently add-and-remove models) will get proportionally more costly as the queue size increases. NP: converting all the users to add-and-forget will consume much more memory, AND will break that assumption of "generally add is to the end of the queue" which is actually not true now anyway. Generally add is to position N-6 of the current queue in front of MemPools garbage collection, disk swap.state cleanup, DelayPools garbage collection, store Digest rebuild, store Digest fetch, peer ICP ping. Maybe in front of a large set of IdlePool timeouts as well. The different length of timer values on each event type above means we cannot easily convert the list type to dlink_list. At least a full analysis of the queue usage. Possibly implementation of both front and back push ops switched by a check for which end is best to inject from (more cycles spent doing the check etc). Amos
Re: [PATCH] Improve Configuration Checking and Parsing
Unless there are any further objections I shall commit this patch tomorrow with the following merge tweaks: * whitespace cleanup via the maintenance script. * narrow down the enable/disable debugs messages to only be about the value found. Cheers Amos
Re: boolean bit fields
On 24/01/2013 7:51 p.m., Alex Rousskov wrote:
On 01/23/2013 07:05 PM, Amos Jeffries wrote:
On 24/01/2013 7:20 a.m., Kinkie wrote:
the attached patch turns the unsigned int:1 flags in CachePeer to
bools.
Please retain the :1 bitmasking. My microbench is showing a consistent
~50ms speed gain on bitmasks over full bool, particularly when there are
multiple bools in the structure. We also get some useful object size gains.
Hello,
FYI: With g++ -O3, there is no measureable performance difference
between bool and bool:1 in my primitive tests (sources attached). I do
see that non-bool bit fields are consistently slower though ("foo:0"
below means type "foo" without bit fields; bool tests are repeated to
show result variance):
Excellent. Thanks for that. I did not go down to the ASM level for my
benchmarks. Just the 100 million loop iteration timing, runs a few
dozens of times to get an idea of the variance.
The binary was not built with -O at all, so whatever the G++ default is
was used.
To me, it looks like bit fields in general may hurt performance where
memory composition is not important (as expected, I guess), and that
some compilers remove any difference between full and bit boolean with
-O3 (that surprised me).
G++ assembly source comparison seem to confirm that -- boolean-based
full and bit assembly sources are virtually identical with -O3 and newer
g++ versions, while bit fields show a lot more assembly operations with
-O0 (both diffs attached). Assembly is well beyond my expertise though.
At -O3 G++ is optimizing for speed at expense of code size.
-O2 is probably a better comparision level and AFAIK the preferred level
for high-performance and small code size build.
Am I testing this wrong or is it a case of YMMV? If it is "YMMV", should
we err on the side of simplicity and use simple bool where memory
savings are not important or not existent?
I think YMMV with the run-time measurement. I had to run the tests many
times to get an average variance range on the speed even at 100M loops.
Some runs the speed was 100ms out in the other direction, but only some,
most were 50ms towards bool:1. And the results differed between flag
position and struct with 1-byte length and struct with enough flags for
2-bytes.
I did not have time to look at the ASM, thank you for the details there.
If -O2 shows the same level of cycles reduction I think I will change my
tack...
we should be letting it handle the bitfields. BUT, we should still
take care to arrange the flags and members such that -O has an easy job
reducing them.
Amos
Re: [PATCH] schedule connect timeouts via eventAdd
On 25/01/2013 12:05 p.m., Rainer Weikusat wrote:
Rainer Weikusat writes:
Alex Rousskov writes:
On 01/24/2013 02:46 PM, Rainer Weikusat wrote:
In my opinion, using
a more sensibly priority queue algorithm (still both extremely simple
to implement and 'readily available' in a canned form), insofar the
deficiencies of the simple one become actually relevant, makes a lot more
sense than devising special-case 'workarounds' for these deficiencies
because it is conjectured that they *might* otherwise become
relevant.
I am all for using better queuing algorithms. However,
[...]
And I'm all for getting some actual work done instead of this
completely pointless discussion.
In case this wasn't clear enough: I'm convinced you're heading down
the wrong road. This conviction may in itself be wrong in some
absolute sense, however, at the moment, I don't think so, and I
cannot possibly justify spending more time argueing back and forth
about it (and wouldn't want to if I could).
Agreed. To cut this all short. I was under the impression that we had
proven the usefulness of add-and-remove model.
Under the current code that 'remove' would be an event delete ("good
enough for now") with possibilities for some future improvement outside
the scope of this change.
So, with maintainer hat on please do the changes necessary to perform
add-and-remove cleanly for timeout events. *If* possible using the
event*() API instead of raw FD table access.
That should be enough to cleanup the timeout problems.
As for connect_retries. It does need some fixes under the new model for
ConnOpener. But that can be a separate patch project and will be
isolated to the Delayed*() callback.
Amos
Re: [PATCH] Improve Configuration Checking and Parsing
On 24/01/2013 4:50 p.m., Amos Jeffries wrote: Unless there are any further objections I shall commit this patch tomorrow with the following merge tweaks: * whitespace cleanup via the maintenance script. * narrow down the enable/disable debugs messages to only be about the value found. Cheers Amos Done. Applied as trunk rev.12611 Amos
Re: boolean bit fields
On 25/01/2013 6:12 a.m., Alex Rousskov wrote: On 01/24/2013 09:27 AM, Kinkie wrote: From what I understand, the current consensus is to use :1'ed bools, right? I hope the consensus is to use full bools (because they are simpler and often faster) except where memory footprint of the structure is both affected _and_ important (a rare case). In those rare cases, use bool:1. HTH, Alex. Yes full bools. I retract my audit requests. Amos
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 25/01/2013 12:40 p.m., Alex Rousskov wrote: On 01/18/2013 04:54 AM, Amos Jeffries wrote: On 18/01/2013 6:47 p.m., Alex Rousskov wrote: Will comm_close() break if we call it instead of reaching into Comm guts and manipulating its tables? If yes, what is Comm missing? Perhaps we can give that missing piece to Comm so that we can comm_close()? Amos, First of all, thank you for taking the time to answer my questions. I believe this issue is an important part of ConnOpener cleanup. * Notice that until ConnOpener has finished the FD is not open, as in "if(isOpen(fd)) return;" produces false, so comm_close() will exit before it gets to any cleanup. AFAICT, Comm marks FD open upon successful comm_openex() exit. ConnOpener even asserts that the temporary FD is flagged as open: // Update the fd_table directly because conn_ is not yet storing the FD assert(temporaryFd_ < Squid_MaxFD); assert(fd_table[temporaryFd_].flags.open); Connection::isOpen() will still return false, but we do not care about that. We just want to call comm_close() as everybody else and comm_close() does not require Connection. * The registered close handler is earlyAbort which flags the FD as error instead of timeout or success. Yes. We would have to remove our close handler before closing if we do not want it to be called when we close, just like all other code using Comm I/O does. The close handler is there for ConnOpener to be notified if some _other_ code closes our FD (e.g., shutdown). * fd_close(temporaryFd_), which we use already, cleans up all the non-ConnOpener state which is associated with the FD. AFAICT, here is some of the other things we are missing by poorly duplicating comm_close() sequence: ++ statCounter.syscalls.sock.closes; /* When one connection closes, give accept() a chance, if need be */ Comm::AcceptLimiter::Instance().kick(); This one might kick us. (sorry bad pun). fdd_table[fd].close_file = file; fdd_table[fd].close_line = line; comm_empty_os_read_buffers(fd); Are these critical? Probably not, but they do invalidate the point that our cleanup code is equivalent to comm_close(). Sooner or later, some of these will come back to hunt us. It is simpler just to prune out the two bits of ConnOpener state in ConnOpener itself rather than rely on the generic close sequence, which has to do a lot of checking for irrelevant things. ConnOpener state should be pruned in ConnOpener, of course, but I think it is clear that we are not doing a good job cleaning Comm state. We duplicate some comm_close() code but missing some details. I agree that comm_close() sequence has many irrelevant things, but it is also used only in a rare case when ConnOpener fails. We do not need to optimize that path! I am just trying to find a way to call comm_close() to clean up Comm state without violating Comm APIs. We will clean the job's state in addition to calling comm_close(). ConnOpener lives in the limbo area between fd_open()/fd_close() which sets up the TCP state on an FD and comm_open()/comm_close() which setup and tear down the mountain of comm transaction I/O state on an FD. I do not see what our temporary FD lacks that the rest of Squid code using comm_open() and comm_close() have. I do not see why it is in that limbo area. Perhaps it was at some point but not anymore? On the other error/timeout exit points from ConnOpener fd_close()+unwinding is the more appropriate way to cleanup [because Comm failed to setup the FD] It did not fail. Comm_openex() was successful. You may indeed be able to re-write comm_close() such that it will work on any FD, including one which failed to reach success in ConnOpener. But I think that is a much more difficult/larger project outside the scope of the priority bug-fixes Rainer is working on here. Perhaps I am missing something, but I do not see why we cannot call comm_close() without any rewrite. We would still need to solve several ConnOpener-specific problems, of course, the ones that are orthogonal to us not properly cleaning up Comm state on failures. IIRC it was just the isOpen() and F->closing() logics that were the problem. If that has gone away like it seems to I have no issue with using comm_close() other than a token hand waving in the direction of performance. Amos
Re: doZeroOnPush
On 24/01/2013 11:36 p.m., Kinkie wrote: On Thu, Jan 24, 2013 at 11:28 AM, Amos Jeffries wrote: On 24/01/2013 5:56 a.m., Kinkie wrote: Hi Amos, what is the best way to signal in a mempools client that the constructor is complete and that thus zeroing is not necessary? It's not immediately evident from looking at the source.. It seems that this is only done in mem.cc for string pools. memDataInit() has an optional bool flag. Set it to false. .. I still can't fully understand how it relates to the usual MEMPROXY_CLASS/MEMPROXY_CLASS_INLINE, and I can't find any good example. -- /kinkie I think I found the answer. And its no good... The objects defined with MEMPROXY_CLASS() macro are all using MemAllocatorProxy::getAllocator() in lib/MemPools.cc to late-initialize their pools. There is no zero flag being passed around in any of those macros or their code paths. So far as I can see to make it optional we will need to make either a template or a new macro which accepts the flag and saves it for use when MemAllocatorProxy::getAllocator().create(...) is done. The doZero() setter needs to be called from the same if() condition right after MemAllocatorProxy::getAllocator().create(). So: 1) - update the macro and roll it out in one step defaulting as (true) everywhere - then set it to false as things get verified. - then eventually remove it again when we have a fully no-zero state for all classes. 2) - add a second macro which sets it to true. - roll out a conversion to the new maro 3) - make a template which can replace MEMPROXY_CLASS() etc. with zero optional flag - rollout the template as we verify classes are constructed properly ?? PS. this also shows that all those classes we made doing memDataInit() late should have been made MEMPROXY_CLASS() children, and memDataInit() formally deprecated. Amos
Re: [PREVIEW] ConnOpener fixes
On 25/01/2013 3:06 p.m., Alex Rousskov wrote: Hello, The attached patch fixes several ConnOpener problems by relying on AsyncJob protections while maintaining a tighter grip on various I/O and sleep states. It is in PREVIEW state because I would like to do more testing, but it did pass basic tests, and I am not currently aware of serious problems with the patch code. I started with Rainer Weikusat's timeout polishing patch posted yesterday, but all bugs are mine. Here are some of the addressed problems: * Connection descriptor was not closed when attempting to reconnect after failures. We now properly close on failures, sleep with descriptor closed, and then reopen. * Timeout handler was not cleaned up properly in some cases, causing memory leaks (for the handler Pointer) and possibly timeouts that were fired (for then-active handler), after the connection was passed to the initiator. * Comm close handler was not cleaned up properly. * Connection timeout was enforced for each connection attempt instead of all attempts together. and possibly other problems. The full extent of all side-effects of mishandled race conditions and state conflicts is probably unknown. TODO: Needs more testing, especially around corner cases. Does somebody need more specific callback cancellation reasons? Consider calling comm_close instead of direct write_data cleanup. Make connect_timeout documentation in squid.conf less ambiguous. Move prevalent conn_ debugging to the status() method? Polish Comm timeout handling to always reset .timeout on callback? Consider revising eventDelete() to delete between-I/O sleep timeout. Feedback welcomed. NP: This is way beyond the simple fixes Ranier was working on. The changes here relys on code behaviour which will limit the patch to trunk or 3.3. I was a bit borderline on the earlier on the size of Raniers patches, but this is going over the change amount I'm comfortable porting to the stable branch with a beta cycle coming to an end. Auditing anyway: * You are still making comments about what "Comm" should do (XXX: Comm should!). ConnOpener *is* "Comm" at this point in the transaction. If "Comm" needs to do anything then it is *this* objects responsibility scope to see that it happens. If thre is a *simple* helper function elsewhere in comm_*() or Comm:: or fd_*() which can help so be it, but this object *is* Comm and needs to peform the "Comm should do X" operations as related to state opening an FD. * It was probably done beforehand, but it much clearer that it happens now that the sleep()/DelayedRetry mechanism leaks Pointer() as well as the InProgress mechanism. +++ IMHO: leave it leaking, the use-case is a rarity and we can update the event API separately and faster than we can fix all the callers to workaround it. * Looking at your comment in there about comm_close() I see why we are at such loggerheads about it. +++ comm_close() does *not* replace the write_data cleanup. - write_data cleanup is explicitly and solely to remove the leaked Pointer()'s *nothing else*. The extra two lines of code are to ensure that hack does not corrupt anything. Until those fd_table pointers stop being dynamic this code is non-optional. - even if you called comm_close() you would need to perform the write_data cleanup before calling it to prevent the leak. * apart from the timeout unset the relevant parts of the comm_close() sequence are all in comm_close_complete(). Perhapse you should schedule one of those calls instead of clearing the handlers and calling fd_table() synchronously.. ++ but notice how that (and comm_close()) adds a second async delay on the FD becoming available for re-use. This is a performance optimization. You will need to setup a synchronous comm_close_complete() to achieve the same speed. ++ the other operations in com_close() function itself are all NOP. No other component has been given the FD or conn_ to set any state themselves. * the naming of "ConnOpener::open()" is ambiguous. Since it is not the active open() operation in this Job. It is the getSocket() operation and should be named as such. Amos
Re: [PATCH] schedule connect timeouts via eventAdd
On 25/01/2013 3:24 p.m., Rainer Weikusat wrote:
Amos Jeffries writes:
[...]
Agreed. To cut this all short. I was under the impression that we had
proven the usefulness of add-and-remove model.
Under the current code that 'remove' would be an event delete ("good
enough for now") with possibilities for some future improvement
outside the scope of this change.
Under normal circumstance, I would have done this 'tomorrow' (whatever
that means, considering that it is unclear for how much longer this
nonsense will keep me awake). Considering that 'the aggressive guy'
was finally made to deliver something (upon the immediate threat that
someone else might do it instead), I'm going to assume that this is a
done deal and won't bother this mailing list anymore except in case of
'other problems' (and even then likely not).
Please don't let this put you off. None of these problems would have
gotten halfway fixed anytime soon if it were not for your valuable time
and effort on this patch.
Alex and I have a lot of differences of opinion as to where the comm
layer scope is exactly and the whole things needs fixing/clarifying. I'm
afraid you happened to get caught in the cross-hairs of that very old
argument and I'm sorry that it has affected your experience with the
Project.
Amos
Re: [PATCH] Move timeout handling from ConnOpener::connect to ConnOpener::timeout
On 25/01/2013 5:34 p.m., Alex Rousskov wrote: On 01/24/2013 06:37 PM, Amos Jeffries wrote: /* When one connection closes, give accept() a chance, if need be */ Comm::AcceptLimiter::Instance().kick(); This one might kick us. (sorry bad pun). Heh! AFAICT, this kick() might deprive us from an FD slot later if we were using the last FD available (and now closing it because of the connect failures that we are hoping to overcome on a retry). I do not think this is a problem because ConnOpener will handle the corresponding conn_openex() "FD limit" error as any other. After all, ConnOpener may run out of FDs even if we did not kick any pending accepts. Did I miss any bad side effects of this kick()? I was referring to the opposite direction - potential problems in the case where a large group of FD would fail doing ConnOpener at roughly the same time but not do the kick(). Leaving queued clients at more risk of timing out on their accept()'s until something else triggered it. IIRC it was just the isOpen() and F->closing() logics that were the problem. If that has gone away like it seems to I have no issue with using comm_close() other than a token hand waving in the direction of performance. Sounds good. Since this is a rare failure case, I do not think we need to optimize it at the expense of code quality. If no other side effects are spotted, and I have the time, I will try comm_close(), which will remove one more instance of direct fd_table manipulation, and a nasty/complex one! ... followed up on the new thread where you posted the patch. Amos
Re: Logging commands: TCP logger vs Logging daemon
On 24/01/2013 8:52 a.m., Alex Rousskov wrote: Hello, I noticed that "tcp://host:port" logger does not get "rotate logs" and other commands that the "daemon:foo" helper gets. The TCP logger just gets the logging line, without any prefix to accommodate future commands or extensions. This seems rather unfortunate because it prevents Squid from reporting significant logging-related events to the remote TCP logger. Log rotation is one example. Another example would be reporting the number of lost (due to connectivity problems) access.log records to a remote logger (if we start supporting such a feature). Is it to late to adjust documentation so that all logging helpers can get logging commands? If it is too late, how about adding an "understands commands" option to access_log so that loggers that can understand commands can receive them? Thank you, Alex. There are at least two users making use of the TCP output method that I am aware of. At this point it woudl be a little messy but still possibly worth doing as a concept. However, this tcp: module is designed as a stream replacement of access.log stdio output all the tools are designed to operate with. Details such as when rotation is requested by the (now external) admin or when Squid starts/stops/reconfigures have never been logged to the access.log files before (udp: and syslog: do not get them either) and cache.log is not sent over the tcp: module. As for lines lost. That might be worth recording. I am thinking a log line " ... lost NNN lines during TCP connection outage ..." would be sufficient ?? Amos
Re: Helpers and Notes
On 26/01/2013 10:39 a.m., Tsantilas Christos wrote:
Hi all,
While we are working in a project we seen that the helpers do wrong
usage of the Notes interface.
This is maybe it is our mistake (and mostly my mistake) because we do
not have make good documentation of this interface.
In any case I believe that we should fix some thinks.
In the Notes.h file there are the following classes:
class Note
---
This is a class used to store a note configuration not a note it self.
It is a note name plus a list of possible values with an ACL list in
each value.
It is used to store configurations for the following cfg lines:
note key value acl1 acl2
class Notes
It is used to store a list of Notes. A list like the following:
note key valueA acl1a acl2a
note key valueB acl1b acl2b
This is used to add custom notes (or metaHeaders) in an HTTP request
With the ACL list being optional and the word "note" being dropped by
the config parser what is actually stored is:
NotesList{
Note{key,value,NULL},
Note{key,value,NULL}
...
}
... which is clearly a list of kv-pairs without any specific information
about delimiter, display semantics, or input source location. Nowhere
in ths implementation or documentation is it clear that this list
structure is *only* ever to be used for parsing the config file. Rather
the simplicity of the information stored implies strongly that it is
created as re-usable storage structure.
***As was requested when you were planning the Notes feature.***
class NotePairs
-
This is an HttpHeader kid class used to store the notes (or
MetaHeaders). We select HttpHeader class because it has some interesting
features. For example assume that two different subsystems add the same
note:
X-SQUID-ProcessedBy: ReWriter
X-SQUID-ProcessedBy: Adaptation
Then this is will be merged to
X-SQUID-ProcessedBy: ReWriter, Adaptation
Yes NotePairs is clearly and specifically a Mime syntax version of kv-pairs.
The interesting and useful fetures are *only* useful when outputting
kv-pairs as ICAP or HTTP headers in Mime format. Those features are not
useful in any other context Squid uses kv-pairs. ... which is why the
helpers avoid that type until the final logging stage where it has no
choice.
Squid helper Notes problems
---
The changes made in trunk revno:12490 and revno:12495
Problems I am seeing:
- it uses class Notes to store the notes/metaHeaders not the NotePairs.
It adds the "Notes HelperReply:notes" and the "Notes
HttpRequest::helperNotes" members.
This is intentional. see above.
The first implementation attempt used a NotePairs list for helpers. The
exact same behaviour of merging keys together which make NotePair
wonderful for ICAP header display and logging display make this NotePair
type difficult to use as a kv-pair list for helpers and any other use
where HTTP field-value syntax is needed.
The other difficulty was the whole or at least a large segment of the
HTTP parser becomes a dependency, which was not worth the cost and
effort for the simplicity of NotePairs. This was probably not noticable
when integrating with ICAP due to it already having that dependency in
place for ICAP header parsing.
The need for multiple de-references ( A->value[X].value()->value ) when
accessing NotesList is accepted as a cost (for now) in exchange for the
benefit of *not* merging key values together and not having to link in
the HTTP parser.
If you are solving problems in the Notes structure systems please
resolve that dereference issue.
- It adds some extra methods which does not make sense and confuse more
the Notes interface:
* NotePairs::append(const Notes::NotesList &src)
* NotePairs::append(const NotePairs *src)
These functions used to add a Notes list to a NotePairs List. The
HttpRequest::helperNotes to AccessLogEntry::notes.
By 'add' you mean 'append list S to this list'. Which is why it is
called 'append()'. Where is the problem? naming, semantics and operation
conform to standard conventions.
The first above converts a NotesList into a NotesPairs list.
Specifically because the NotePairs list is hard-coded to utilize the
MIME format key-value pairs with colon (:) delimiter - which in Squid
are stored in HttpHeader class types.
The second is added such that any existing AccessLogEntry list can be
appended to by other sources producing NotePairs lists (ie RESPMOD).
But It is easier to use NotePairs for HttpRequest::helperNotes member
and just use the (pre-)existing NotePairs::append (the HttpHeaders::append)
Causing the need to covert a key=value string pair into an HttpHeader
object. Causing the need to define said HttpHeader as a custom header
and fill out its value set. All of which adds HTTP protocol operations
(and validity checks) for the sake of parsing the very non-HTTP input
"OK foo=hello".
Not to mention that "foo_
Re: [PREVIEW] ConnOpener fixes
On 26/01/2013 6:35 a.m., Alex Rousskov wrote: On 01/24/2013 09:35 PM, Amos Jeffries wrote: On 25/01/2013 3:06 p.m., Alex Rousskov wrote: NP: This is way beyond the simple fixes Ranier was working on. The changes here relys on code behaviour which will limit the patch to trunk or 3.3. I was a bit borderline on the earlier on the size of Raniers patches, but this is going over the change amount I'm comfortable porting to the stable branch with a beta cycle coming to an end. I have not looked at v3.2 specifically during this project, so I do not know which behaviors make my patch incompatible with that version. I certainly do not insist that you port it to v3.2, but I am happy to help with that if you decide that it is the best way forward. * You are still making comments about what "Comm" should do (XXX: Comm should!). ConnOpener *is* "Comm" at this point in the transaction. Fixed comments by using specific Comm API names. * It was probably done beforehand, but it much clearer that it happens now that the sleep()/DelayedRetry mechanism leaks Pointer() Where do you see a leak? Please note that our static DelayedRetry event callback is always called, regardless of the job state/existence, so it will always delete the Pointer. I clarified that point by adjusting the comment in cancelSleep(). You commented earlier that the events were scheduled as AsyncCall queue. You commented earlier that AsyncCall queue would not run a Call if the Job were gone. I connected those two statements to imply the DelayRetry event would not happen. Same as is happening for InProgress. Uhm, were you meaning these events are *always* run and the scheduling happens in our wrapper which deletes the Pointer() ? If so there is no leak. Otherwise there is. * Looking at your comment in there about comm_close() I see why we are at such loggerheads about it. +++ comm_close() does *not* replace the write_data cleanup. Fixed comments that were missing two facts: 1) ConnOpener's bypass of Comm::Write() API makes COMMIO_FD_WRITECB(fd)->active() false, which means comm_close() will not call and cleanup the write handler properly. Sorry for missing that point. We should consider using Comm::Write eventually so that comm_close can do this cleanup for us instead (and so that we can use regular non-leaking job callbacks). That does ring a memory bell. Sorry I can't recall specifics right now though. Good catch. On the whole I am kind of against calling comm_close() for the sheer number of conditionals it requires to effectively do a NOP operation. It is nicer from the code readability and completeness viewpoint, but not performance. Even a few dozen micro-seconds in the ConnOpener and related stages can translate to a significatn proportion of a MEM_HIT transaction time. 2) comm_close() is probably slightly broken itself because its COMMIO_FD_WRITECB cleanup condition is probably wrong. It should not matter whether there is an active callback when it comes to the SetSelect cleanup call. That call should be done unconditionally, and some SetSelect call implementations may optimize to clean only when there _was_ a write handler at some point. We have fixed a related bug #3048 (r10880) by adding those cleanup calls, but it looks like we should have been even more aggressive and make cleanup unconditional. This may explain why I see many "FD ready but there is no handler" debug lines in some cache.logs! If I am correct, fixing this may even speed things up a little as we would be polling fewer FDs :-). Happy to review the patch when you have it ready. :-) * apart from the timeout unset the relevant parts of the comm_close() sequence are all in comm_close_complete(). Not exactly. There is more potentially useful code in current _comm_close(): source code address setting and the arguably less useful comm_empty_os_read_buffers() call. And who knows what else will be added or changed there! We should call comm_close() instead of trying to take it apart and use what we think is currently relevant. IMO this is basic API sanity policy: If we call comm_open, we must call comm_close. Okay. However I am trying to work towards an overall goal of reducing the things comm_close() does, by moving the cleanup of these things into the Jobs which are setting them. * the naming of "ConnOpener::open()" is ambiguous. Since it is not the active open() operation in this Job. It is the getSocket() operation and should be named as such. Fixed by renaming open() to createSocket(). I wanted to emphasize that this method creates something rather than just extracting something that was there before. Fair enough. I'm fine with that. Adjusted patch attached. Besides method renaming and comment changes discussed above, this patch * Calls comm_close() as discussed. * No longer resets COMM_SELECT_WRITE when keeping FD for the job initiator, to mimic Comm
Re: doZeroOnPush
On 26/01/2013 6:56 a.m., Alex Rousskov wrote: On 01/24/2013 07:27 PM, Amos Jeffries wrote: On 24/01/2013 11:36 p.m., Kinkie wrote: On Thu, Jan 24, 2013 at 11:28 AM, Amos Jeffries wrote: On 24/01/2013 5:56 a.m., Kinkie wrote: Hi Amos, what is the best way to signal in a mempools client that the constructor is complete and that thus zeroing is not necessary? It's not immediately evident from looking at the source.. It seems that this is only done in mem.cc for string pools. memDataInit() has an optional bool flag. Set it to false. .. I still can't fully understand how it relates to the usual MEMPROXY_CLASS/MEMPROXY_CLASS_INLINE, and I can't find any good example. -- /kinkie I think I found the answer. And its no good... The objects defined with MEMPROXY_CLASS() macro are all using MemAllocatorProxy::getAllocator() in lib/MemPools.cc to late-initialize their pools. There is no zero flag being passed around in any of those macros or their code paths. So far as I can see to make it optional we will need to make either a template or a new macro which accepts the flag and saves it for use when MemAllocatorProxy::getAllocator().create(...) is done. The doZero() setter needs to be called from the same if() condition right after MemAllocatorProxy::getAllocator().create(). So: 1) - update the macro and roll it out in one step defaulting as (true) everywhere - then set it to false as things get verified. - then eventually remove it again when we have a fully no-zero state for all classes. I think this is the best option, especially if you trust Coverity to eventually find all initialization problems. It comes with virtually no initial risk and results in very few code changes. 2) - add a second macro which sets it to true. - roll out a conversion to the new maro OK, but I think #1 is better because #1 results in fewer code changes. I may change my opinion if Kinkie's tests prove that fewer doZero buffers result in significantly better performance, justifying per-class, incremental changes. 3) - make a template which can replace MEMPROXY_CLASS() etc. with zero optional flag - rollout the template as we verify classes are constructed properly Doable, but too complex IMO, without much added benefit that I can see. Complex? Do you have any examples I can take a look at? Just for my edification. I will take your word for it not being worthwhile persuing for now. Amos
Re: [PATCH] No-lookup DNS ACLs
This patch has passed the 10-day standown period and has no audit objects. I only note that it uses class member dynamic array definitions (foo[];) definitions. The C++ feature will break build on some compilers we need to support (clang for FreeBSD9). Once that is fixed this patch can go in. Amos On 25/12/2012 9:08 a.m., Tsantilas Christos wrote: No-lookup DNS ACLs Currently, dst, dstdom, dstdom_regex (and other?) DNS-related ACLs do DNS lookups if such a lookup is needed to convert an IP address into a domain name or vice versa. This creates two kinds of problems: - It is difficult to identify requests that use raw IP addresses in Request-URI or Host headers. One would have to use something like url_regex and possibly req_header to identify those before using dst ACLs to match the request destination against a known IP subnet. IPv6 would only make this harder. - It is difficult to use dst* ACLs in options that support fast ACLs only. If an async lookup is required, the answer will be unpredictable (now) or DUNNO (when the ACL bugs are fixed), possibly with warnings and other complications. This patch adds a -n option to dst, dstdom, dstdom_regex and other DNS-related ACLs. The option disable lookups and address type conversions. If lookup or conversion is required because the parameter type (IP or domain name) does not match the message address type (domain name or IP), then the ACL with a -n option would immediately declare a mismatch without any warnings or lookups. Please note that -n prohibits lookups in Squid's DNS caches as well. This patch also adds an ACL flags mechanism to help us easily add new flags for acls. The supported flags for an acl type configured in ACL constructor. Extra care taken for the -i/+i regex flags. These flags are not normal flags because they can be applied everywhere in acls values: acl dstdomain_regex -i dom1 dom2 +i dom3 -i dom4 This is a Measurement Factory project. Regards, Christos
Re: Helpers and Notes
On 27/01/2013 8:27 a.m., Tsantilas Christos wrote:
On 01/26/2013 06:05 AM, Amos Jeffries wrote:
On 26/01/2013 10:39 a.m., Tsantilas Christos wrote:
Hi all,
While we are working in a project we seen that the helpers do wrong
usage of the Notes interface.
This is maybe it is our mistake (and mostly my mistake) because we do
not have make good documentation of this interface.
In any case I believe that we should fix some thinks.
In the Notes.h file there are the following classes:
class Note
---
This is a class used to store a note configuration not a note it self.
It is a note name plus a list of possible values with an ACL list in
each value.
It is used to store configurations for the following cfg lines:
note key value acl1 acl2
class Notes
It is used to store a list of Notes. A list like the following:
note key valueA acl1a acl2a
note key valueB acl1b acl2b
This is used to add custom notes (or metaHeaders) in an HTTP request
With the ACL list being optional and the word "note" being dropped by
the config parser what is actually stored is:
NotesList{
Note{key,value,NULL},
Note{key,value,NULL}
...
}
Well, not actually. What it is stored is the following:
NoteList {
Note {key, Values{{value1, NULL}, NULL} }
Note {key, Values{{value1, NULL}, NULL} }
...
}
A Note keep a note name plus a list of values/ACLs. The Acls used to
select the correct value for a Note.
It is designed to store configuration not a note pair.
Moreover to get the Value of a Note a new method added the
Notes::firstValue() to retrieve the first value of the list. It is not
so cool. It is a little confusing...
... which is clearly a list of kv-pairs without any specific information
about delimiter, display semantics, or input source location. Nowhere
in ths implementation or documentation is it clear that this list
structure is *only* ever to be used for parsing the config file. Rather
the simplicity of the information stored implies strongly that it is
created as re-usable storage structure.
***As was requested when you were planning the Notes feature.***
Of course it can be used to store kv-pairs. What I am saying is that the
NotePairs class is better structure to keep this list.
class NotePairs
-
This is an HttpHeader kid class used to store the notes (or
MetaHeaders). We select HttpHeader class because it has some interesting
features. For example assume that two different subsystems add the same
note:
X-SQUID-ProcessedBy: ReWriter
X-SQUID-ProcessedBy: Adaptation
Then this is will be merged to
X-SQUID-ProcessedBy: ReWriter, Adaptation
Yes NotePairs is clearly and specifically a Mime syntax version of
kv-pairs.
The interesting and useful fetures are *only* useful when outputting
kv-pairs as ICAP or HTTP headers in Mime format. Those features are not
useful in any other context Squid uses kv-pairs. ... which is why the
helpers avoid that type until the final logging stage where it has no
choice.
So we are agree that the NotePairs class is more useful to keep kv-pairs
at least for the ICAP/HTTP headers. But this is does not mean that it
can not be used for storing helpers kv-pairs.
It is just better to use the same structures for storing similar
information. It is better to just use NotePairs for storing notes,
because it can handle more cases (kv-pairs, ICAP/eCAP metaheaders and
maybe other type of notes in the future)
Also storing all notes in the same object maybe it is good policy
because they can be used in other than the logging cases.
Currently all notes merged just before logging because they are used
only here.
Squid helper Notes problems
---
The changes made in trunk revno:12490 and revno:12495
Problems I am seeing:
- it uses class Notes to store the notes/metaHeaders not the NotePairs.
It adds the "Notes HelperReply:notes" and the "Notes
HttpRequest::helperNotes" members.
This is intentional. see above.
The first implementation attempt used a NotePairs list for helpers. The
exact same behaviour of merging keys together which make NotePair
wonderful for ICAP header display and logging display make this NotePair
type difficult to use as a kv-pair list for helpers and any other use
where HTTP field-value syntax is needed.
Why?
The other difficulty was the whole or at least a large segment of the
HTTP parser becomes a dependency, which was not worth the cost and
effort for the simplicity of NotePairs. This was probably not noticable
when integrating with ICAP due to it already having that dependency in
place for ICAP header parsing.
You do not have to use any HTTP parser. You can just add a kv-pair (a
key/value pair), simply using the NotePairs::putExt(key, value).
Why the Notes::add(key, value) is better?
The need for multiple de-references ( A->value[X].value()->value ) when
accessing NotesList is accepted as
Re: [PATCH] No-lookup DNS ACLs
On 28/01/2013 12:04 a.m., Tsantilas Christos wrote:
On 01/26/2013 12:11 PM, Amos Jeffries wrote:
This patch has passed the 10-day standown period and has no audit objects.
I had forgot it. Thank you.
I only note that it uses class member dynamic array definitions (foo[];)
definitions. The C++ feature will break build on some compilers we need
to support (clang for FreeBSD9).
Are you referenced to the definitions like the followings?
ACLFlag DestinationDomainFlags[] = {ACL_F_NO_LOOKUP, ACL_F_END};
const ACLFlag ACLFlags::NoFlags[] = {ACL_F_END};
Will the clang work if I convert them to:
ACLFlag DestinationDomainFlags[64] = {ACL_F_NO_LOOKUP, ACL_F_END};
const ACLFlag ACLFlags::NoFlags[64] = {ACL_F_END};
or even better use the following:
typedef ACLFlag ACLFlagsSupported[64];
ACLFlagsSupported DestinationDomainFlags = {ACL_F_NO_LOOKUP, ACL_F_END};
const ACLFlagsSupported ACLFlags::NoFlags = {ACL_F_END};
Opinion on this?
No these all have size and definitions.
Sorry should not have used plural. The problem is likely to be:
static const ACLFlag NoFlags[];
defined as [] but with no size.
Amos
Once that is fixed this patch can go in.
Amos
On 25/12/2012 9:08 a.m., Tsantilas Christos wrote:
No-lookup DNS ACLs
Currently, dst, dstdom, dstdom_regex (and other?) DNS-related ACLs do
DNS lookups if such a lookup is needed to convert an IP address into a
domain name or vice versa. This creates two kinds of problems:
- It is difficult to identify requests that use raw IP addresses in
Request-URI or Host headers. One would have to use something like
url_regex and possibly req_header to identify those before using dst
ACLs to match the request destination against a known IP subnet. IPv6
would only make this harder.
- It is difficult to use dst* ACLs in options that support fast ACLs
only. If an async lookup is required, the answer will be unpredictable
(now) or DUNNO (when the ACL bugs are fixed), possibly with warnings and
other complications.
This patch adds a -n option to dst, dstdom, dstdom_regex and other
DNS-related ACLs. The option disable lookups and address type
conversions. If lookup or conversion is required because the parameter
type (IP or domain name) does not match the message address type (domain
name or IP), then the ACL with a -n option would immediately declare a
mismatch without any warnings or lookups.
Please note that -n prohibits lookups in Squid's DNS caches as well.
This patch also adds an ACL flags mechanism to help us easily add new
flags for acls. The supported flags for an acl type configured in ACL
constructor.
Extra care taken for the -i/+i regex flags. These flags are not normal
flags because they can be applied everywhere in acls values:
acl dstdomain_regex -i dom1 dom2 +i dom3 -i dom4
This is a Measurement Factory project.
Regards,
Christos
Re: Helpers and Notes
On 28/01/2013 6:32 a.m., Tsantilas Christos wrote:
On 01/27/2013 07:12 AM, Amos Jeffries wrote:
On 27/01/2013 8:27 a.m., Tsantilas Christos wrote:
On 01/26/2013 06:05 AM, Amos Jeffries wrote:
On 26/01/2013 10:39 a.m., Tsantilas Christos wrote:
I did not found such cases in the existing code.
What I am seeing is the following usage:
Note::Pointer statusNote = reply.notes.find("status");
...
const char * result = statusNote->firstValue();
status = (http_status) atoi(result)
...
urlNote = reply.notes.find("rewrite-url");
if (urlNote != NULL && strcmp(urlNote->firstValue(), http->uri)
Which requires two local Pointers to access the value.
firstValue() is the accessor Alex had me add to hide the worst part of
the de-referencing.
Without it we would have:
reply.notes.find("status")->values[0]->value.termedBuf()
where it is preferrable to go:
reply.notes.find("status")->firstValue();
The above if we use NotePairs instead of Notes can be easily written as
follows:
String status = reply.notes.getByName("status");
status = (http_status) atoi(status.termedBuf())
Your example works for numeric values because ',' is not a valid digit
and atoi() will abort before parsing the already parsed values.
It gets worse when handling URLs or any other value type which coudl
ontain whitespace or ',' values internally:
1)
helper reply: OK url=http://example.com/ url=http://invalid.invalid/
translates to a single URL. String uri = reply.notes.getByName("uri");
will produce URL "http://example.com/, http://invalid.invalid/"; which
the lenient URL parser (default behaviour) will see as
"http://example.com/,%20http://invalid.invalid/";.
Is it valid for a helper to return more than one kv-pairs with the same
key?
Yes and no.
For some of the planned keys like group= and tag= (maybe store-id=), it
is expected that there will be duplicates. For others like status= and
url=, redirect-url=, it is expected that they will be used once. The
code with firstValue() is writen so that only the first value matters
and any duplicates are ignored until logging.
The current code does not permit it. It will take care only for the
first kvpair. It adds multiple values to the same note key, but it uses
only the first.
They are permitted at present but not used.
The HelperReply parser scope does not have enough information to do the
duplicate checking, so the higher level callback handler it delivers the
details to is left to cope with duplicates.
So if no more than one values should used then the HelperReply.cc code
should not add the second "url=" kvpair. It is simple.
The HelperReply.cc code does not know which interface the helper is
using. Multiple url= parameters are valid (as unused annotations) on all
the auth helpers interfaces. But not so much on the URL helper interfaces.
If we are going to support more than kv-pairs with the same key then the
HTTP header like behavior is very good choice.
See also my comment bellow for the "tag=".
2)
helper reply: OK user="J, Smith" group="group1,A" group=group2
user=alias group=A
String group --> "group1,A, group2, group3" ... how does one
determine that a group match is supposed to be equal to "group1,A" and
not match "group1" or "A" individually, BUT must match "group2" or "B"
individually?
String user ---> "J, Smith, alias" ... what username is this? it won'
match any, and splitting on ',' will also likey not match any. But "J,
Smith" could match.
Again the current code does not support multiple kv-pairs with the same
key.
The helper coders can use different tokens for example "J.Smith" or
"group1+A"
Also see my comment bellow for the "tag=" case.
The tokens are coming straight from the actual user/password/group names
from whatever backend the network is using. We have zero control over
what they contain.
On delivery to Squid the value is de-coded into those original plain
binary characters (UTF-8? ISO-8859-1?) before being added to the notes
kv-pair list. We can't count on the ',' staying coded in some
helper-controlled encoding for its whole usage through Squid.
3)
helper reply: OK tag=1 tag=something tag=magic
Will produce a single tag String "1, something, magic". What does the
tag ACL type do?
What the tag currently does?
Good question. Thanks you for this question.
Assume that you have one ACL helper. You want with one lookup to this
ACL helper to take 3-4 decisions. For example:
1) Do or not sslbump
2) Send or not to one or more ICAP services for example to an antivirus
service.
If I am not wrong (please fix me) currently the helper can return:
ta
[PATCH] SNMP defects from Coverity
Coverity has detected a long list of small defects in the snmp_core.cc code.
Lots of alignment, sizeof(), and memory uninitialized.
They all stem from a few uses of "sizeof(name)" where name is a pointer
to an OID object rather than the object itself. This code is present as
far back as squid-2.6 and probably a lot further.
I believe it has not been causing obvious problems due to the
sizeof(oid*) [4 or 8 octets] on a pointer being larger than the
sizeof(oid) [1 or 2 octets] on the object stored into the allocate memory.
Is anyone able to test the attached patch please?
Amos
=== modified file 'src/snmp_core.cc'
--- src/snmp_core.cc2013-01-27 17:35:07 +
+++ src/snmp_core.cc2013-01-27 23:48:59 +
@@ -705,8 +705,8 @@
{
oid *instance = NULL;
if (*len <= current->len) {
-instance = (oid *)xmalloc(sizeof(name) * (*len + 1));
-memcpy(instance, name, (sizeof(name) * *len));
+instance = (oid *)xmalloc(sizeof(*name) * (*len + 1));
+memcpy(instance, name, sizeof(*name) * (*len));
instance[*len] = 0;
*len += 1;
}
@@ -722,8 +722,8 @@
int index[TIME_INDEX_LEN] = {TIME_INDEX};
if (*len <= current->len) {
-instance = (oid *)xmalloc(sizeof(name) * (*len + 1));
-memcpy(instance, name, (sizeof(name) * *len));
+instance = (oid *)xmalloc(sizeof(*name) * (*len + 1));
+memcpy(instance, name, sizeof(*name) * (*len));
instance[*len] = *index;
*len += 1;
} else {
@@ -733,8 +733,8 @@
++loop;
if (loop < (TIME_INDEX_LEN - 1)) {
-instance = (oid *)xmalloc(sizeof(name) * (*len));
-memcpy(instance, name, (sizeof(name) * *len));
+instance = (oid *)xmalloc(sizeof(*name) * (*len));
+memcpy(instance, name, sizeof(*name) * (*len));
instance[*len - 1] = index[++loop];
}
}
@@ -761,8 +761,8 @@
instance = client_Inst(current->name, len, current, Fn);
} else if (*len <= current->len) {
debugs(49, 6, "snmp peer_Inst: *len <= current->len ???");
-instance = (oid *)xmalloc(sizeof(name) * ( *len + 1));
-memcpy(instance, name, (sizeof(name) * *len));
+instance = (oid *)xmalloc(sizeof(*name) * ( *len + 1));
+memcpy(instance, name, sizeof(*name) * (*len));
instance[*len] = 1 ;
*len += 1;
} else {
@@ -773,8 +773,8 @@
if (peers) {
debugs(49, 6, "snmp peer_Inst: Encode peer #" << i);
-instance = (oid *)xmalloc(sizeof(name) * (current->len + 1 ));
-memcpy(instance, name, (sizeof(name) * current->len ));
+instance = (oid *)xmalloc(sizeof(*name) * (current->len + 1 ));
+memcpy(instance, name, (sizeof(*name) * current->len ));
instance[current->len] = no + 1 ; // i.e. the next index on
cache_peeer table.
} else {
debugs(49, 6, "snmp peer_Inst: We have " << i << " peers. Can't
find #" << no);
@@ -808,8 +808,8 @@
debugs(49, 6, HERE << "len" << *len << ", current-len" << current->len
<< ", addr=" << laddr << ", size=" << size);
-instance = (oid *)xmalloc(sizeof(name) * (*len + size ));
-memcpy(instance, name, (sizeof(name) * (*len)));
+instance = (oid *)xmalloc(sizeof(*name) * (*len + size ));
+memcpy(instance, name, (sizeof(*name) * (*len)));
if ( !laddr.IsAnyAddr() ) {
addr2oid(laddr, &instance[ *len]); // the addr
@@ -832,8 +832,8 @@
debugs(49, 6, HERE << "len" << *len << ", current-len" <<
current->len << ", addr=" << laddr << ", newshift=" << newshift);
-instance = (oid *)xmalloc(sizeof(name) * (current->len +
newshift));
-memcpy(instance, name, (sizeof(name) * (current->len)));
+instance = (oid *)xmalloc(sizeof(*name) * (current->len +
newshift));
+memcpy(instance, name, (sizeof(*name) * (current->len)));
addr2oid(laddr, &instance[current->len]); // the addr.
*len = current->len + newshift ;
}
[RFC] HTTP problems caused by broken ICAP servers
Alex and co., I'd like your feedback on this please... The issue outlined very clearly in this squid-users mail: http://www.squid-cache.org/mail-archive/squid-users/201301/0358.html is that ICAP server was broken and producing a stupid response. On 1/28/2013 11:24 AM, Michael Graham wrote: ICAP/1.0 200 OK Server: Traffic Spicer 2.2.2 ISTag: "Bloxx/v2.2.2/c5edeb8/vBLOXX" Encapsulated: req-hdr=0, req-body=789 This is the problem here. This should be null-body=789. If I fix the icap response so that we say null-body then the first squid doesn't add the Transfer-Encoding header, which the second squid then converts into a Content-Length: 0 header. Now that the problem is identified, I can recall it being debugged by Alex multiple times in the past as well for other users. Can we add some better protocol idiocy detection on the ICAP responses please? I know Squid is faithfully replicating what ICAP tol it to produce. But we need something to flag when ICAP is telling Squid to produce a risky HTTP syntax combination. Or something the Squid admin has configured (or defaulted) to not be possible. Thanks Amos
Re: Sometimes, old principles might still be good.
On 29/01/2013 11:50 p.m., Reiner Karlsberg wrote: I am new here, so please be kind :-) In case, the reader considers "Egoless Programming" an "out of date" principle, then simply ignore this mail. But in case, the reader of this mail can detect some interesting issues, a discussion about it is welcome. Just one issue: what are you on about? - Rock code not being up to our own coding guidelines? or something else? Amos
Re: eventAdd with zero delay?
On 30/01/2013 2:47 p.m., Rainer Weikusat wrote: What's the point of putting these onto the event queue? Wouldn't this be better done with something like this? The Call queue and the Events queue are drained individually in alternating fashion. The first 0-delay event will not run until after the Call queue is empty. The Call queue by design lacks anything similar to the 'heavy' event loop exit point. So an explicit 0-delay event is used to break heavy sequences of Calls into separate chains. Using ScheduleCall immediately would be counter-productive in many of these cases. If you can identify 0-delay events which should actually be scheduled immediately patches converting them to Call's are welcome. Amos
Re: Partial contenet,206
On 30/01/2013 2:07 p.m., Eliezer Croitoru wrote: This topic was discused couple times and I want to get back to it with some highlights. Since we want to maximize cache efficiency we must apply something that will allow partial content caching. The situation now is kind of all or nothing. One of the reasons for that alone is that squid stores objects in a way that dosn't allow cached files/objects manipulation. The cache objects are in kind of RO mode but this might not be an issue for all solutions that can be offered to solve partial content caching. Things we should be considering how much do we want to gain from partial contenet caching? a 8kb cache gain 512kb and more? Does not matter. For starters we want to be *able* to store any cacheable response. do we want a perfeft object in cache? We do. But definition of 'perfect' when it comes to Ranges... we want the bytes to be prefectly matching the servers copy, even if we are storing a sub-range of the whole. Extreme care taken to validate that two ranges are from the exact same variant before merging, etc. To store them separately if there is any doubt. or partial data can be inserted to a object? We want that too of course. The best proposal I've heard put forward years ago was to store the headers meta data and the object entity in separate cache files and when storing ranges to open the file size and fill only the stored part. Possibly with background fetches as low-priority traffic to fetch the other ranges and make it perfect for future requests. * we already have range_offset_limit and quick_abort settings to control these. Will probably require some adjustment in what they do and/or new directives. * the background fetch part of this is stuck behind a re-design of the async fetch code in 3.x (also blocking Collapsed Forwarding [see Alex]). NP: this whole topic is probably best looked at after the Collapsed Forwarding feature is built into 3.x. The background async requests for Ranges will need to be collapsable in case any client wants to do its own range filling (ie client Squids). I dont know how cache like varnish or orhers do this. any ideas? I was thinking of asking the varnish developers about their approach to the subject. Anorher issue is that the cache object id (store-id)is decided before any response is being recived from the server. This can also be solved in a way but first before any other step please feel free to throw anything you have.(i know there was another thread in the past) Amos
Re: Propose to polish the consistency of case sensitivity in configurations
On 30/01/2013 9:21 a.m., Alex Rousskov wrote: On 01/29/2013 01:49 AM, Tianyin Xu wrote: 3. For configuration like A B=C, A and B should be sensitive while C should be insensitive. The case sensitivity of squid.conf directive parameters (i.e., all the stuff that follows the directive name) depends on the directive. Many C values are case-sensitive (e.g., various locations). However, you should not see a lot of str*cmp() code applied to those values -- if that is how you plan locating code for planned consistency fixes. We can probably declare all known names such as directive names and directive parameter names case-insensitive. That would make their handling consistent and backward compatible. However, that "permissive" direction feels inconsistent with recent changes that, among other things, restricted the variation of "positive" keywords such as "on", "enabled", and "yes". Other than consistency with some existing options, I do not see value in making configuration names case-insensitive (unless required so by some communication protocol, of course). Variations in case just make configs messier IMO. If we were to start from scratch, I would argue that all known configuration names should be case sensitive. I find it interesting that squid.conf.documented does not document default case sensitivity. Perhaps we can use that to justify making everything sensitive by default? :-) It seems to have started as all case-sensitive then been moved to insensitive in bits and pieces as users complained about the strictness, Their mixed-case configs resulting in "Bungled config" when to a human it reads "right". I'm of the opinion we should be strict in what we want, loose in what we accept, and liberal with annoying warnings when they don't follow the party line. NP: changing the case-sensitivity on directive names is an all-or-nothing choice, since the directive parser functions are auto-generated. Amos
Re: scalable event scheduling
On 30/01/2013 10:31 a.m., Rainer Weikusat wrote: Rainer Weikusat writes: [...] There are probably all kinds of 'weird, stylistic issues' and possible, yet unknown bugs, Attached is a slightly updated version of this. Mostly, it fixes an accidentally inverted comparison in down_heap, uses the squid x* memory allocation wrapper routines I found meanwhile and is more careful to avoid possibly invoking external code while the heap is not in a consistent state. It has still only been used on development computers. Couple of things stand out... * if tag is always a class ev_tag, why is it being cast to/from void* all over the place? there is almost no need for void* to exist in C++. Just pre-define class ev_tag; and use "ev_tag &" parameter types (as const wherever possible). * The code you are replacing uses the convention of return value type on a separate line to the function name. Please follow that style. * I know the code you are replacing uses C-style functions for each action. But please make things like eventCancel() members of the event object being cancelled. Particularly since you are already testing whether that object exists before cancelling, there is no extra cost of correcting the function scope. * Also, can you get any performance numbers about the impact of the change please? ... still reading the changes, so there may be more. But that seems big enough for now. Amos
Re: scalable event scheduling
On 31/01/2013 6:28 a.m., Alex Rousskov wrote: On 01/30/2013 09:30 AM, Rainer Weikusat wrote: Alex Rousskov writes: On 01/28/2013 03:29 PM, Rainer Weikusat wrote: so easy that even using the STL implementation wouldn't be worth the effort Can you clarify why we should not just use one of the STL queues instead? I agree with your opinion that "the STL doesn't contain an 'obviously good' choice for a priority queue implementation in the given context". I am glad we agree on this, but I think that STL must be seriously considered before we add something custom. The overhead of making that decision was one of the reasons why I considered optimizing event queue a stand-alone, non-trivial project. I see no reason to optimize Squid event queue now, but if the consensus is that it should be optimized, I think we should evaluate usability of standard STL queues as the first step. the event scheduler will store a value at the pointed-to location which can be passed to eventDelete to cancel a pending event 'safely' in logarithmic time, meaning, even if the event was possibly already put onto the async queue. The purpose of the eventDelete routine is not to cancel a call the event scheduler already put onto the async call queue but to prevent such a call from being put onto this queue if this is still possible. And I didn't intend change that. Noted, but I am still missing something: Why add a new event tag API if the existing eventDelete() code can already cancel events outside the async queue? [ If the new tag API is needed, and if the cancellation API is documented, please clarify that only events currently outside the async queue can be canceled.] I do not think such "occasionally working" cancellation is a good idea though. The caller code would have to take different actions depending on whether the event was actually canceled or not. And, in some cases, the correct "not canceled" action would be difficult to implement as the ConnOpener use case shows. I also think that the need for explicit destruction of the event "tag" by the caller is a serious downside because it increases the risk of memory leaks. FWIW, here is how I think the event cancellation should be supported instead: 1. Another form of eventAdd() should be added to accept caller's async call. New code, especially async jobs, should use that form (jobs will and avoid call wrappers that way). Old code that wants to cancel events should use this new form as well. I've kind of gone off the idea of leaving old code using a wrapper API with the old behaviour. All that we have got out of that approach is added technical debt for others whoc have to ome along afterwards and learn two API interfaces then decide which ne to remove (sometimes picking the wrong one, witness my own ongoing use of parse_*() parser API for additions and doZero change in the wrong MemPools interface). It is far better, easier, simpler, safer to have the one person who understands the change properly, replace all the existing API fuctions in one polish patch. This will also teach them if there is some weird caller hidden somewhere that breaks under the new design. AND, will assist with portage by causing build errors when the API from newer code is not converted to the older versions required API. Other than that I agree with this approach to AsyncCall conversion of eventAdd(). It is the same plan I had settled on. BUT this is not the same project Rainer is working on (replacing the engine, not the API). 2. The caller may store and cancel that async call at any time prior to its firing, just like any other async callback. 3. When the event is ready for the async queue, the event scheduler should put the same async call from #1 into the async queue. The current event scheduler already creates an async call object for nearly all events anyway, so this does not add CPU overhead. The only overhead is that we will consume the same [small] amount of RAM sooner. In exchange, we get guaranteed call cancellation, job protection, and improved debugging. Alex. P.S. I have also considered the following alternative, but rejected it because to use event cancellation new caller code would have to be written anyway, so we do not need to preserve the current eventAdd() parameters for that new code: 1. eventAdd() should create, store, and return an async call. 2. The caller may store and cancel the returned async call at any time prior to its firing, just like any other async callback. 3. When the event is ready for the async queue, the event scheduler should put the stored async call into the async queue. Amos
Re: Partial contenet,206
On 31/01/2013 2:11 a.m., Eliezer Croitoru wrote: On 1/30/2013 8:48 AM, Amos Jeffries wrote: Does not matter. For starters we want to be *able* to store any cacheable response. Since I dont know the exact progress of collapse forwarding. The request contains the desire to fetch partial data so now the only thing is to somehow store and re-serve it. If I remember right the vary headers are being used to create the store-id of the request. What is actually being done when forcing full response rather then partial? What I was pointing about the store-id being decided before getting any data is for what i'm about to write. What happens in a case when a user requests partial content and the server denies it and returns full response? squid cache that or not? Yes, the same caveats about merging with existing cached Ranges apply except that this should *replace* the whole cached Range entry with the new object instead of merging. Merge should only happen when two Ranges are supposedly the same object. Maybe using overlapping bytes to help verify they are identical. A nice example for partial content that is distributed on multiple objects is YOUTUBE and other videos. One video can be composed from partial parts of one video. There are situations which some client fastforward to specific place in the video but still the player works with the chunks intelligently enough that only some of the chunks are out of the fixed size of a chunk. So eventually it's ok. Have you actually sighted use of the Range: HTTP headers in Youtube traffic? and have you verified that (2) beow is no longer happening (as of 2 year ago it was)? In my experience ... 1) they do not use HTTP Range: headers on most of the videos (at least 2 years ago it was rare). 2) they prefix each snippet of video with FLV meta headers 3) several of the HD video encodings used are VBR encodings based on IP-layer speed detection. Together all these details mean that no two YouTube "range" replies can be merged together. They best one can hope for is that two clients will make identical requests. But given that they are generated by the user clicking along the video timeline bar the likelyhood of seeing that happen in one proxy is too small to bother coding for. Also, serving a proper HTTP Range reply by slicing a previously stored video would loose that nasty FLV meta header. Effectively corrupting the players view of teh response. So we can might try to store partial responses in a cache object specifically for partial response. This is where my question was about size since most of the clients will request more then just 8k chunk and we can even do some statistics from existing live server logs to make sure of it. This is the most practical way in the current situation. This indeed will open a small weakness in cases which there are two clients requesting the same object one with partial content request and the other a full response. A solution for that can be for squid to learn if it dosn't know how to serve partial content from a complete file. Squid already has this capability. For the range_offset_limit and HIT Range: serving capabilities. This is mainly why I was suggesting wait on Collapsed Forwarding, because you might find it easier to collapse the two requests down and service the Range: request out of the full requests response. Or service the full request using two parallel 'filler' requests around the underway Range request - together the three background requests consisting of a full object fetch. Amos
Re: scalable event scheduling
On 31/01/2013 3:12 p.m., Rainer Weikusat wrote: Alex Rousskov writes: On 01/30/2013 09:30 AM, Rainer Weikusat wrote: Alex Rousskov writes: On 01/28/2013 03:29 PM, Rainer Weikusat wrote: so easy that even using the STL implementation wouldn't be worth the effort Can you clarify why we should not just use one of the STL queues instead? I agree with your opinion that "the STL doesn't contain an 'obviously good' choice for a priority queue implementation in the given context". I am glad we agree on this, but I think that STL must be seriously considered before we add something custom. It is not suitable in this case and reading through the documentation should also show that as the comparable 'STL thing' lacks a 'delete any object' interface. I'm also no more convinced of the merits of the STL approach than the people who wrote (or didn't change) all the other existing code which could theoretically use it, up to the linked-list based events.cc itself. [...] the event scheduler will store a value at the pointed-to location which can be passed to eventDelete to cancel a pending event 'safely' in logarithmic time, meaning, even if the event was possibly already put onto the async queue. The purpose of the eventDelete routine is not to cancel a call the event scheduler already put onto the async call queue but to prevent such a call from being put onto this queue if this is still possible. And I didn't intend change that. Noted, but I am still missing something: Why add a new event tag API if the existing eventDelete() code can already cancel events outside the async queue? I already explained this in my first mail on this topic and I will gladly answer (reasonable) questions based on what I wrote there. [...] I do not think such "occasionally working" cancellation is a good idea though. There are two different queueing subsystems involved here and because of this, users of the code will have to deal with both. This may be regarded as unfortunate design choice but in any case, it wasn't my choice[*]. This is a good argument for doing the event AsyncCall conversion before the engine conversion. That way the cancellation from the outside perspective is a uniform AsyncCall cancellation whichever engine is used. We just end up with a (brief?) period of inefficiency as we use a linked-list filled with events waiting to schedule already cancelled Calls. This is not a big problem IMO. Both are relatvely small simple steps when taken individually. [*] In order to get stuff done, one has to start with something reasonably simple that it can be accomplished relatively easily on its own and deal with the remaining deficits of 'the universe in the pitiful state it generally happens to be in' afterwards. At least, it will be a somewhat less pitiful state then. [...] I also think that the need for explicit destruction of the event "tag" by the caller is a serious downside because it increases the risk of memory leaks. No more serious than all the existing situations where this is already necessary, including kernel objects like file descriptors [*]. IMHO the cancellation should be an optimization not a memory leak prevention. Proper use of the Pointer patterns please in all new code where it makes sense. FWIW, here is how I think the event cancellation should be supported instead: 1. Another form of eventAdd() should be added to accept caller's async call. New code, especially async jobs, should use that form (jobs will and avoid call wrappers that way). Old code that wants to cancel events should use this new form as well. This would imply keeping cancelled events on the event scheduler queue until they expire 'on their own' because the metainformation necessary to remove them efficiently from this queue is not available in this way. That's another thing I used to do in the past but the amount of code which doesn't need to be written because of this (essentially, a single if-statement) doesn't warrant the runtime overhead of not providing this functionality, especially in the case of a 'universal' timer subsystem which is both useful for tasks which are usually expected to be executed and tasks which usually won't run (like timeouts). A better idea would be to add an interface which takes a call instead of the function pointer + argument pointer as argument so that all options would be available to be used whenever their use happens to make sense (an even better idea would be to integrate these two separate things in some 'closure-like' object and to relieve users from the burden of worrying about two pointers instead of one but that's sort of a digression). Uhm, that is what Alex just said in the first sentence of (1). The implication of leaving events queued until they expire only stands for old code which did that already. Old code which needs to remove events should be updated to holding the Call they scheduled in the event queue, and cancelling it. This is a go
Re: [PATCH] ConnOpener fixes
On 29/01/2013 6:32 a.m., Alex Rousskov wrote: Hello, The attached patch fixes several ConnOpener problems by relying on AsyncJob protections while maintaining a tighter grip on various I/O and sleep states. This patch is identical to the last ConnOpener patch posted in PREVIEW state. I am now done with valgrind testing. No leaks were detected, although I doubt my tests were comprehensive enough to trigger all possible conditions where the unpatched code leaks. I started with Rainer Weikusat's timeout polishing patch posted a few days ago, but all bugs are mine. Here are some of the addressed problems: * Connection descriptor was not closed when attempting to reconnect after failures. We now properly close on failures, sleep with descriptor closed, and then reopen. * Timeout handler was not cleaned up properly in some cases, causing memory leaks (for the handler Pointer) and possibly timeouts that were fired (for then-active handler) after the connection was passed to the initiator. * Comm close handler was not cleaned up properly. * statCounter.syscalls.sock.closes counter was not updated on FD closure. * Waiting pending accepts were not kicked on FD closure. * Connection timeout was enforced for each connection attempt instead of applying to all attempts taken together. and possibly other problems. The full extent of all side-effects of mishandled race conditions and state conflicts is probably unknown. TODO (outside this project scope): Polish comm_close() to always reset "Select" state. Make connect_timeout documentation in squid.conf less ambiguous. Move prevalent conn_ debugging to the status() method? Polish Comm timeout handling to always reset .timeout on callback? Revise eventDelete() to delete between-I/O sleep timeout? HTH, Alex. One performane optimization I can see: * if the callback_ is != NULL but has been cancelled by the receiving Job. Your code now calls sendAnswer() and schedules the already cancelled Call. Previously this was tested for in swanSong() to simply NULL the callback_ pointer instead of scheduling a dead Call. NP: it would probably be a good idea to test for the callback_->cancelled() case and debugs() report it instead of scheduling. The result of omitting this check saves one if() test per connection by adding the full overheads of memory, scheduling, and CPU for one AsyncCall queue operation per connection. ConnOpener operates on the server-side where multiple client-side Jobs may result in TIMEOUT or ABORTED. This cancellation case is a small but common case. +1. Other than the above performance tweak this looks fine for commit. Amos
Re: scalable event scheduling
On 31/01/2013 6:44 p.m., Alex Rousskov wrote: On 01/30/2013 06:30 PM, Amos Jeffries wrote: I've kind of gone off the idea of leaving old code using a wrapper API with the old behaviour. All that we have got out of that approach is added technical debt for others whoc have to ome along afterwards and learn two API interfaces then decide which ne to remove I agree that adding a conflicting API is bad, so I would like to add a fourth step to my sketch: 4. Upgrade all users of eventDelete(f,a) API that currently supply a non-NULL second argument to use new eventDelete() that supplies an async call (from step #1). If possible, convert all other eventDelete users to the new eventDelete(). If not possible, rename the old two-argument eventDelete(f,NULL) to a single-argument eventDeleteAll(f) call. This fourth step will eliminate or, in the worst case, minimize doubts and conflicts for future API users. It is far better, easier, simpler, safer to have the one person who understands the change properly, replace all the existing API fuctions in one polish patch. This will also teach them if there is some weird caller hidden somewhere that breaks under the new design. AND, will assist with portage by causing build errors when the API from newer code is not converted to the older versions required API. Agreed. Unfortunately, it is often the case that the "one person who understands the change properly" is nowhere to be found, too busy, is viewed as disrupting attempts to fix something by others, and/or multiple people think they understand the change properly, but their understanding differs :-). Interesting definition of "one" you have there. I was speaking about the time of initial API change. The points you highlight as issues down the track are all caused by broken or inomplete API transitions, which a lot of the time are caused directly by doing the transition with wrappers. Amos
Re: [RFC] Peek and Splice
On 1/02/2013 5:17 p.m., Alex Rousskov wrote: Hello, Many SslBump deployments try to minimize potential damage by _not_ bumping sites unless the local policy demands it. Unfortunately, this decision must currently be made based on very limited information: A typical HTTP CONNECT request does not contain many details and intercepted TCP connections are even worse. We would like to give admins a way to make bumping decision later in the process, when the SSL server certificate is available (or when it becomes clear that we are not dealing with an SSL connection at all!). The project is called Peek and Splice. The idea is to peek at the SSL client Hello message (if any), send a similar (to the extent possible) Hello message to the SSL server, peek at the SSL server Hello message, and then decide whether to bump. If the decision is _not_ to bump, the server Hello message is forwarded to the client and the two TCP connections are spliced at TCP level, with Squid shoveling TCP bytes back and forth without any decryption. If we succeed, the project will also pave the way for SSL SNI support because Squid will be able to send client SNI info to the SSL server, something that cannot be done today without modifying OpenSSL. I will not bore you with low-level details, but we think there is a good chance that Peek and Splice is possible to implement without OpenSSL modifications. In short, we plan using OpenSSL BIO level to prevent OpenSSL from prematurely negotiating secure connections on behalf of Squid (before Squid decides whether to bump or splice). We have started writing BIO code, and basic pieces appear to work, but the major challenges are still ahead of us so the whole effort might still fail. There are a few high-level things in this project that are not clear to me. I hope you can help find the best solutions: 1. Should other bumping modes switch to using SSL BIO that is required for Peek and Splice? Pros: Supporting one low-level SSL I/O model keeps code simpler. Cons: Compared to OpenSSL native implementation, our BIO code will probably add overheads (not to mention bugs). Is overall code simplification worth adding those overheads and dangers? 2. How to configure two ssl_bump decisions per transaction? When Peek and Splice is known to cause problems, What problems and how would this be known? the admin should be able to disable peeking using CONNECT/TCP level info alone. Thus, we probably have to keep the current ssl_bump option. We can add a "peek" action that will tell Squid to enable Peek and Slice: Peek at the certificates without immediately bumping the client or server connection (the current code does bump one or the other immediately). However, many (most?) bumping decisions should be done when server certificate is known -- the whole point behind Peek and Splice. We can add ssl_bump2 or ssl_bump_peeked that will be applied to peeked Please avoid "ssl_bump2". transactions only: ssl_bump peek safeToPeek ssl_bump none all ssl_bump_peeked server-first safeToBump ssl_bump_peeked splice all Is that the best configuration approach, or am I missing a more elegant solution? What about making "peek" a flag that can optionally prefix any of the ssl_bump ACL actions... ssl_bump client-first foo !safeToPeek ssl_bump peek client-first foo ssl_bump server-first !safeToPeek ssl_bump peek server-first safeToBump ssl_bump none all Overall the whole thing is getting complex and risks making a huge mess. Time to reduce the options. This *is* a quazi-legal feature after all. Amos
Re: Squid 3.3 countdown
ETA for 3.3.1 as of this writing is 9th Feb. That said, we still have over 60 unresolved major bugs from older versions affecting 3.3. Amos
Re: int to intXX_t conversion
On 2/02/2013 8:27 a.m., Alex Rousskov wrote: On 02/01/2013 12:06 PM, Kinkie wrote: BTW: c++11 defines intXX_t as part of the core language; I believe that to be an indication that the language will evolve in that direction. I sure hope C++ does not evolve in the direction of uint64_t! :-) Just because something very useful is added, does not mean it should be used by default instead of something old (but also very useful). Agreed. The only reason I'm aware of having the intXX_t in the guidelines at all is that there is a mess of alternative intXX labeling styles out there and we needed to pick one for consistent size indication - and followed the C++XX styling at the time. It is to be used when size matters, BUT, whenever the size does not matter we can leave it to the older more generic naming. IMO we will one day want to be converting a lot of those to "auto" instead of a specific sized int anyway. Yes these do need doing in a lot of places to ensure the 64-bit systems work properly. However it is obviously far from a quick process. Many of the areas needing 64-bit changes will also need som eextensive testing done at the same time. Kinkie, for the quick renaming projects I think the object name and static/non-static member capitalization styles would be a good next target. Followed by private member _ naming. Amos
Re: Is it a loop bug or not? Sorry missing part.
On 2/02/2013 1:42 p.m., Eliezer Croitoru wrote: On 2/2/2013 2:35 AM, Eliezer Croitoru wrote: I was trying to access my /squid-internal-mgr/* and it seems to have a problem. The visible host name is www1.home ip 192.168.10.1 I have the proper acls to allow manager access and I get: 1359765266.436 10002 192.168.10.100 TCP_MISS/408 0 GET http://www1.home:3128/squid-internal-mgr/menu - HIER_NONE/- text/html And I get The connection was reset. Sorry missing part. When I am doing it using as forward proxy and use the url to the intercept port 3127 i'm getting into a loop: accessing: http://www1.home:3127/squid-internal-mgr/menu 1359765678.173 88894 192.168.10.100 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - 1359765678.269 88966 127.0.0.1 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - sme miss abort for a very very long time =\ Ah. Interesting. The pattern is that it is supposed to be just the visible_hostname value plus the internal manager path. When you add port it breaks the visible_hostname to URL matching and Squid relays it onwards to what it thinks is the origin server. You should have the intercept port listened on by Squid firewalled so direct connections to it cannot succeed. If you are using DROP to do that you will see these timeouts, if you are using REJECT you will get a fast fail result. If you don't have it firewalled properly the lopo detectino in Squid should kick in. PS. we had a proposal a while back to to visible_hostname matching per listening port. But this breaks forwarding loop detection a bit. Amos
Re: [PATCH] SNMP defects from Coverity
On 28/01/2013 2:07 p.m., Amos Jeffries wrote: Coverity has detected a long list of small defects in the snmp_core.cc code. Lots of alignment, sizeof(), and memory uninitialized. They all stem from a few uses of "sizeof(name)" where name is a pointer to an OID object rather than the object itself. This code is present as far back as squid-2.6 and probably a lot further. I believe it has not been causing obvious problems due to the sizeof(oid*) [4 or 8 octets] on a pointer being larger than the sizeof(oid) [1 or 2 octets] on the object stored into the allocate memory. Is anyone able to test the attached patch please? Amos With no objections, this is applied to trunk a rev.12641. Amos
Re: [squid-users] what should squid -z do
On 4/02/2013 7:54 a.m., Eliezer Croitoru wrote: On 2/3/2013 8:29 PM, Alex Rousskov wrote: The cause of this specific problem is_not_ rock behavior (correct or not) but a mismatch between a startup script and squid.conf -- the script does not test for all the right directories when running squid-z. Even if rock code is changed, this mismatch still needs to be fixed. Please consider filing a bug report with Debian/Ubuntu if it is their fault. The problem _is_ rock behaviour. Because it is initializing without checking for existence. Erasing a working DB if the admin happens to run squid -z without understanding its full operation. -z should be able to run at any time without any major consequences. I am completely in favour of adjusting -z to perform a validate by default when possible. However, that does not change the fact that rock is currently performing an _erase_ operation when one exists. To be consistent with ufs, we should probably change rock behavior from initializing the database to doing nothing if the configured database directory already exists. Like ufs, rock will rely on directory existence for that test (regardless of what may be inside that configured directory). In other words, squid-z is not "validate and initialize" but "initialize if the configured directory is not there". Any objections to that rock change? Absolutely none. Start by making both perform an existence check before doing *anything*. * initialize if it does not exist yet * optionally perform validation + repair if something does. UFS seems to scan the directory structure and create missing sub-directories. Omiting the file corruption tests. We should probably add these now that they are possible. Rock should probably scan the potentially corrupt database and try to drop/erase individual corrupt entries before deciding whether to completely re-initialize the DB. My starter assumption was that squid -z erase or reset any cache_dir. Then I found out it's not like that. The init scripts checks for directories AND FILES but is not smart enough to verify the integrity of the content. So now the question is: If squid has the ability to verify the cache_dir structure and DB more then an init script, Why do we even let the script decide this kind of decision? Init scripts do this because Squid crashes on startup if the directories do not exist, squid-2 packaged with a default cache_dir, and some systems are automatically built using tools like puppet - drop config files into /etc/default/$PROG ; install $PROG ; service start $PROG. Less than 5 minutes from bare metal to operational server. Squid in any case rebuilds ufs stores or fix if corrupted or not?? right? "not". This operation is a todo list entry. Why squid should not create a cache_dir if one dosn't exits at startup? What side effects can come from that? It can more complex but a "check", "reset", "build" flags can be added to the -z like in -k parse|...|..| while having a default to "build" which is what it does's now. The "build" will be the default and compatible with the current -z flag works. The "check" can respond to an init or any other script a more informative way about the check like 1 is bad 2 is something dirty 3 there is not store present 4 DB corrected. I just think loudly since the subject was opened.
Re: [PATCH] Make squid -z for cache_dir rock work like UFS, not COSS
On 5/02/2013 8:48 a.m., Alex Rousskov wrote: On 02/03/2013 10:06 PM, Alex Rousskov wrote: On 02/03/2013 08:31 PM, Amos Jeffries wrote: On 2/3/2013 8:29 PM, Alex Rousskov wrote: To be consistent with ufs, we should probably change rock behavior from initializing the database to doing nothing if the configured database directory already exists. Like ufs, rock will rely on directory existence for that test (regardless of what may be inside that configured directory). In other words, squid-z is not "validate and initialize" but "initialize if the configured directory is not there". Any objections to that rock change? Absolutely none. Start by making both perform an existence check before doing *anything*. * initialize if it does not exist yet Will do. The attached trunk patch makes squid -z for cache_dir rock work like UFS instead of like COSS. When a startup script runs squid -z by mistake against a cache_dir that is already initialized and full of cached entries, some admins prefer that nothing happens. Rock store now skips reinitialization if both the cache_dir directory and the db file in that directory exist. If one or both are missing, the missing pieces are created. UFS does something similar because it creates missing L1 and L2 directories but does not erase any entries already present in the cache_dir path. COSS, OTOH, re-initializes the existing db (but nobody noticed or complained loud enough, apparently). Rock behavior will now be closer to UFS. To clean a corrupted cache_dir, the admin must remove it before running squid-z. +1. Although when comitting can you add a few words to the -z description in main.cc usage() function and src/squid.8.in man file, so that it is clear this only a create-if-missing action. Amos
[PATCH] Bug 3686 - refactor the config options controlling max object size
http://bugs.squid-cache.org/show_bug.cgi?id=3686
The maximum cached object size gets capped to the highest specified
max-size of any cache_dir, even if there is cache_dir available with no
max-size specified.
The solution:
Re-document the global as a default limit with cache_dir specific values
overriding it for that store. And re-implement it strictly as a default
only applied when a cache_dir has no explicit max-size setting.
After this patch:
- maximum_object_size is converted into the default value for cache_dir
max-size= options when none is supplied.
- every cache_dir will always produce a max size for that storage area.
This is accessible as StoreDir::maxObjectSize().
- no cache_dir may be configured with a larger max-size= than will fit
in that storage area.
- cache_dir may be configured with max-size=0 which will result in no
new additions being made, *as well* as any form of revalidation purging
existing entries as they are detected to be "too big" for the area.
NP: use no-store option to make a cache_dir read-only without this
purging behaviour.
- Squid will not cache any object larger than the *largest* storage
maximum (including memory cache maximum_object_size_in_memory). This is
currently accessible as store_maxobjsize.
- if no cache_dir are specified, the largest cached object will be
limited by maximum_object_size_in_memory.
Amos
=== modified file 'src/SwapDir.cc'
--- src/SwapDir.cc 2013-01-31 15:05:34 +
+++ src/SwapDir.cc 2013-02-05 05:39:40 +
@@ -42,8 +42,8 @@
#include "tools.h"
SwapDir::SwapDir(char const *aType): theType(aType),
-max_size(0),
-path(NULL), index(-1), disker(-1), min_objsize(0), max_objsize (-1),
+max_size(0), min_objsize(0), max_objsize (-1),
+path(NULL), index(-1), disker(-1),
repl(NULL), removals(0), scanned(0),
cleanLog(NULL)
{
@@ -114,6 +114,38 @@
return ((maxSize() * Config.Swap.lowWaterMark) / 100);
}
+int64_t
+SwapDir::maxObjectSize() const
+{
+// per-store max-size=N value is authoritative
+if (max_objsize > -1)
+return max_objsize;
+
+// store with no individual max limit is limited by configured
maximum_object_size
+// or the total store size, whichever is smaller
+return min(static_cast(maxSize()), Config.Store.maxObjectSize);
+}
+
+void
+SwapDir::maxObjectSize(int64_t newMax)
+{
+// negative values mean no limit (-1)
+if (newMax < 0) {
+max_objsize = -1; // set explicitly in case it had a non-default value
previously
+return;
+}
+
+// prohibit values greater than total storage area size
+// but set max_objsize to the maximum allowed to override
maximum_object_size global config
+if (static_cast(newMax) > maxSize()) {
+debugs(47, DBG_PARSE_NOTE(2), "WARNING: object 'max-size' option for "
<< path << " cannot exceed " << maxSize());
+max_objsize = maxSize();
+return;
+}
+
+max_objsize = newMax;
+}
+
void
SwapDir::reference(StoreEntry &) {}
=== modified file 'src/SwapDir.h'
--- src/SwapDir.h 2013-01-31 15:05:34 +
+++ src/SwapDir.h 2013-02-03 13:36:43 +
@@ -146,9 +146,15 @@
virtual uint64_t maxSize() const { return max_size;}
+/// the minimum size of singel object which may be stored here
virtual uint64_t minSize() const;
-virtual int64_t maxObjectSize() const { return max_objsize; }
+/// The maximum size of single object which may be stored here.
+int64_t maxObjectSize() const;
+
+/// configure the maximum object size for this storage area.
+/// May be any size up to the total storage area.
+void maxObjectSize(int64_t newMax);
virtual void getStats(StoreInfoStats &stats) const;
virtual void stat (StoreEntry &anEntry) const;
@@ -180,13 +186,13 @@
protected:
uint64_t max_size;///< maximum allocatable size of the storage area
+int64_t min_objsize; ///< minimum size of any object stored here (-1
for no limit)
+int64_t max_objsize; ///< maximum size of any object stored here (-1
for no limit)
public:
char *path;
int index; /* This entry's index into the swapDirs array */
int disker; ///< disker kid id dedicated to this SwapDir or -1
-int64_t min_objsize;
-int64_t max_objsize;
RemovalPolicy *repl;
int removals;
int scanned;
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2013-02-04 09:47:50 +
+++ src/cache_cf.cc 2013-02-05 04:08:33 +
@@ -277,16 +277,23 @@
static void
update_maxobjsize(void)
{
-int i;
int64_t ms = -1;
-for (i = 0; i < Config.cacheSwap.n_configured; ++i) {
+// determine the maximum size object that can be stored to disk
+for (int i = 0; i < Config.cacheSwap.n_configured; ++i) {
assert (Config.cacheSwap.swapDirs[i].getRaw());
-if (dynamic_cast(Config.cacheSwap.swapDirs[i].getRaw())->
-
Re: [PATCH] StoreId with couple cosmetics in sync with trunk 1639
This audit round all appear to be documentation an debugs polish. Any objection to me applying those changes and comitting? Amos
Re: [PATCH] StoreId with couple cosmetics in sync with trunk 1639
On 8/02/2013 5:23 a.m., Alex Rousskov wrote: On 02/06/2013 08:45 PM, Amos Jeffries wrote: This audit round all appear to be documentation an debugs polish. Any objection to me applying those changes and comitting? None from me, especially if you can remove code duplication that Eliezer cannot (the still unaddressed issue from the previous review). Thank you, Alex. All done, including the de-duplication, and applied as trunk rev.12655. Please test the changes to redirect.cc are still working. Amos
Re: [PATCH] if-else for announce_period
On 8/02/2013 10:07 p.m., Tianyin Xu wrote:
It's not wrong but when I see the code I want to refine it since it
looks uncomfortable.
680 if (Config.Announce.period > 0) {
681 Config.onoff.announce = 1;
682 } else if (Config.Announce.period < 1) {
683 Config.Announce.period = 86400 * 365; /* one year */
684 Config.onoff.announce = 0;
685 }
Announce.period is with type "time_t" so it's an integer. The
"else-if" is "logically" superfluous because as long as the value <=0,
it must be <1.
+1 and applied to trunk. Thank you.
Amos
Re: [PATCH] Bug 3686 - refactor the config options controlling max object size
On 6/02/2013 8:14 a.m., Alex Rousskov wrote:
On 02/05/2013 03:36 AM, Amos Jeffries wrote:
http://bugs.squid-cache.org/show_bug.cgi?id=3686
+if (static_cast(newMax) > maxSize()) {
+debugs(47, DBG_PARSE_NOTE(2), "WARNING: object 'max-size' option for " << path <<
" cannot exceed " << maxSize());
+max_objsize = maxSize();
+return;
+}
Please adjust this to say "cannot exceed total cache_dir size " or
similar to clarify where the limit is coming from.
I would also say "ignoring max-size for because it exceeds
cache_dir " instead of the "max-size cannot exceed ..." phrase, to
tell the admin what Squid is doing instead of just telling them what
Squid cannot do. This will make it clear that their max-size option has
no effect.
In summary:
debugs(47, DBG_PARSE_NOTE(2), "WARNING: ignoring max-size for " <<
path << " because it exceeds total cache_dir size " << maxSize());
+/// the minimum size of singel object which may be stored here
"single" typo
I would rephrase as "smaller objects will not be added and may be purged".
Actually this was completely wrong anyway. minSize() *is* the function
paired with maxSize() and seems to be related to the low watermark somehow.
Dropped the documentation change.
-virtual int64_t maxObjectSize() const { return max_objsize; }
+/// The maximum size of single object which may be stored here.
+int64_t maxObjectSize() const;
Same rephrasing suggestion as the above.
Please do not remove "virtual". It is a useful reminder that this is a
part of the Store API.
Why add virtual table entries? this is not a replaceable function any
more. This is an accessor method for the SwapDir base object.
Also, I think it would be appropriate (and useful) to document maxSize()
method in this patch because it looks symmetrical to now-documented
minSize() but has a very different meaning.
N/A now and I'm not sure I understand it well enough to add documentation.
=== modified file 'src/peer_digest.cc'
--- src/peer_digest.cc 2013-01-30 15:39:37 +
+++ src/peer_digest.cc 2013-02-03 13:38:13 +
@@ -347,6 +347,7 @@
req->header.putStr(HDR_ACCEPT, "text/html");
+// XXX: this is broken when login=PASS, login=PASSTHRU, login=PROXYPASS, login=NEGOTIATE, and login=*:...
if (p->login)
xstrncpy(req->login, p->login, MAX_LOGIN_SZ);
An unrelated change?
Er. yes. dropped.
StoreEntry *const pe = addEntry(i);
+printf("pe->swap_status == %d (SWAPOUT_WRITING=%d)\n", pe->swap_status, SWAPOUT_WRITING);
+
CPPUNIT_ASSERT(pe->swap_status == SWAPOUT_WRITING);
CPPUNIT_ASSERT(pe->swap_dirn == 0);
CPPUNIT_ASSERT(pe->swap_filen >= 0);
Perhaps remove the added debugging to reduce "make check" noise? Or make
it conditional on swap_status being not SWAPOUT_WRITING?
+rep->setHeaders(HTTP_OK, "dummy test object", "x-squid-internal/test", 0,
-1, squid_curtime + 10);
You may want to add a line to the commit message to note why this change
was necessary (and that test cases now have weak/indirect checks for
max-size code).
All of the above can be done during commit, without another review cycle
IMO.
Thank you,
Alex.
Applie dto trunk as rev.12662.
Amos
Re: [PATCH] Bug 3686 - refactor the config options controlling max object size
On 9/02/2013 5:58 p.m., Alex Rousskov wrote:
On 02/08/2013 05:54 PM, Amos Jeffries wrote:
-virtual int64_t maxObjectSize() const { return max_objsize; }
+int64_t maxObjectSize() const;
Please do not remove "virtual". It is a useful reminder that this is a
part of the Store API.
Why add virtual table entries? this is not a replaceable function any
more. This is an accessor method for the SwapDir base object.
Store, the SwapDir's parent class has the "virtual" keyword for this
method. Thus, the method will be virtual in all child classes and will
be in the virtual table regardless of whether we say "virtual" explicitly:
Store.h:virtual int64_t maxObjectSize() const = 0;
The above Store class API gets implemented by these Store children:
MemStore.h: virtual int64_t maxObjectSize() const;
StoreHashIndex.h: virtual int64_t maxObjectSize() const;
SwapDir.h:virtual int64_t maxObjectSize() const;
SwapDir.h:int64_t maxObjectSize() const;
The reason I believe it is better to say "virtual" explicitly in this
case is to remind us that maxObjectSize() is a part of the [semi-broken]
Store API and not some SwapDir "own" method. This will help us when we
polish the API to remove the common Store root.
Again, being explicit here does not add or remove any virtual table entries.
Hope this clarifies,
Alex.
That makes sense. Followup patch applied.
Amos
Re: Squid 3.3 countdown
On 2/02/2013 12:01 a.m., Kinkie wrote: Hi all, I'm done with the bool-ification effort (only pending is the TrafficMode merge from Amos). This is now done. But the PortCfg flags are still not bools. It seems you will need to re-do that part of it. Amos
Re: [PATCH] StoreId with couple cosmetics in sync with trunk 12639
On 11/02/2013 9:18 a.m., Eliezer Croitoru wrote: On 2/8/2013 11:46 AM, Amos Jeffries wrote: On 8/02/2013 5:23 a.m., Alex Rousskov wrote: On 02/06/2013 08:45 PM, Amos Jeffries wrote: This audit round all appear to be documentation an debugs polish. Any objection to me applying those changes and comitting? None from me, especially if you can remove code duplication that Eliezer cannot (the still unaddressed issue from the previous review). Thank you, Alex. All done, including the de-duplication, and applied as trunk rev.12655. Please test the changes to redirect.cc are still working. Amos Thanks Amos, The code looks good and I managed to rebuild squid from trunk 12669. I will test it in the next few days to see if there are side effects. The next goals are: - enabling storeId for ICP.(async etc..) - enabling multi store-id key-val with first being used unless there is an storeId which already cached. - enabling storeId in REQMOD ICAP interface.(sending current storeId to the helper) Any other suggestions\request are welcome? Portage of this bit to 3.3 ? That requires an alteration of the redirect.cc code handling helper reply syntax since the OK/ERR/BH and kv-pairs don't exit there. storeId is in 3.4 now and we can hapilly leave it for release there if you like. Just be aware I'm expecting that to only go stable late this year. I noticed another issue regarding squid with kids process: when used with two cache_dirs(rock+aufs) it uses two kids, is it suppose to be like that? Yes Squid splits into a Disker kid which manages all the disk I/O processes and a Worker which handles all the HTTP proxying and most other tasks. Amos
Re: [PATCH] Refactor mime.cc
On 11/02/2013 6:02 a.m., Kinkie wrote:
Hi all,
the attached patch aims to c++-ify and standardize a bit more mime.cc
and the auxiliary classes it contains.
No functional changes.
It'd be nice to change the mime table from being a table of regexes to
a suffix search - after all, all mime extensions recorded there are
suffixes except for the catch-all case.
This code is however not on a critical path, so I'd leave that step
post-stringng.
--
/kinkie
Since this is a refactor please fix the coding guidelines differences as
well. I have added some here:
* private variables suffixed with '_' (ie MimeIcon::url_, MimeIcon::icon_)
* please make MimeEntry a MEMPROXY class. Possibly also MimeIcon.
* please remove empty lines added at the end of MimeEntry class definition.
* with MimeIcon url and icon members being private you are free to
update to String from char*. Which will remove the need for all the
char* management code inside that object.
* in MimeIcon::created there are debugs() being wrapped which need not
be. Later on there are setHeaders in the same position.
* MimeEmtry destructor opening { needs to be on the line following the
function name.
* please add a Mime:: namespace, and make the global function static
functions inside it, or members of one of the classes.
Amos
Re: [PATCH] Coverity issue 740457
On 12/02/2013 1:37 a.m., Kinkie wrote: * the string created by tempnam() apparently needs to be xfree()'d when done. doh! Ok. * please add a comment in the #else condition noting the tempnam() is officially obsolete from POSIX-2008.1. (in a few more years we should drop the #else code entirely.) Done. we may want to shuffle this to compat/ somewhere. * please also add a note that tmpfile() is not an option since we desire the /tmp file to stick around after process exit for administrators use. Do we? There's an unlink(filename) call at the end of that function As a followup change we need to consider that this whole process leaves a /tmp file sitting around from every crash, which could over time build up. I'm in two minds about how desirable this behaviour is. Generally /tmp is either a ramdisk or there are system tools to clean accumulated cruft up. I wouldn't worry about this. + we could check for the files on startup and do something when Squid is re-started after a crash. + administrator can manually find the file even if the mail attempt failed (known to happen). - the files build up over time and could be a big waste of disk space. See above. Very well. +1. Amos
Re: Is it a loop bug or not? Sorry missing part.
On 11/02/2013 9:13 a.m., Eliezer Croitoru wrote: On 2/2/2013 6:23 AM, Amos Jeffries wrote: On 2/02/2013 1:42 p.m., Eliezer Croitoru wrote: On 2/2/2013 2:35 AM, Eliezer Croitoru wrote: Sorry missing part. When I am doing it using as forward proxy and use the url to the intercept port 3127 i'm getting into a loop: accessing: http://www1.home:3127/squid-internal-mgr/menu 1359765678.173 88894 192.168.10.100 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - 1359765678.269 88966 127.0.0.1 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - sme miss abort for a very very long time =\ Ah. Interesting. The pattern is that it is supposed to be just the visible_hostname value plus the internal manager path. When you add port it breaks the visible_hostname to URL matching and Squid relays it onwards to what it thinks is the origin server. You should have the intercept port listened on by Squid firewalled so direct connections to it cannot succeed. If you are using DROP to do that you will see these timeouts, if you are using REJECT you will get a fast fail result. If you don't have it firewalled properly the lopo detectino in Squid should kick in. PS. we had a proposal a while back to to visible_hostname matching per listening port. But this breaks forwarding loop detection a bit. Amos I have tried trunk for the next rules as a safety and it seems to work fine. ##start conf http_port 0.0.0.0:3127 intercept name=intercept http_port 0.0.0.0:3128 http_port 0.0.0.0:3129 tproxy name=tproxy acl intercept_ports myportname intercept tproxy http_access deny manager intercept_ports http_access allow manager localhost http_access deny manager ##end conf The main problem is that squid tries to connect the local intercept port. In this case specifically I can use iptables to block traffic from localhost to localhost on the dst port of 3127 or 3128 but it stil causes and almost endless loop that tries to connect again and again not related to iptables but to squid loop prevention. I think that squid should be by default able to detect a loop with this specific "character". And as an example to what it does: 1360526890.476 262596 127.0.0.1 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - 1360526890.803 262921 127.0.0.1 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - 1360526891.139 263254 127.0.0.1 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - 1360526891.474 263575 127.0.0.1 TCP_MISS_ABORTED/000 0 GET http://www1.home:3127/squid-internal-mgr/menu - HIER_DIRECT/127.0.0.1 - I think this logs talks for itself pretty well. The only ways to stop squid from retrying these is by reload or restart. but reloading is not stopping the main issue which is the mem + FD + cpu consumption(various situations). What do you think? A warning in docs is good enough or fixing it? Regards, What are via and visible_hostname configured to in your Squid? (or in your /etc/resolv.conf for hostname) Amos
Re: Is it a loop bug or not? Sorry missing part.
On 13/02/2013 12:01 a.m., Eliezer Croitoru wrote: On 2/12/2013 1:25 AM, Amos Jeffries wrote: What are via and visible_hostname configured to in your Squid? (or in your /etc/resolv.conf for hostname) Amos visible_hostname www1.home via off Ah. Squid detects loops by looking for its visible host name in Via: header. So the loop is "normal" as far as loops go. If you re-enable Via it should stop looping at least. The main bug seems to be in that the /squid-internal part of the URL is not being caught and serviced. It should have been identified and Squid generate a response before the request got anywhere near looping. Looking into it now. If you want to parallel my search it should be the client_side_request.cc processRequest or such. # hostname www1.home #/etc/resolve.conf dosn't contains any domain related info just the local cache dns server. #/etc/hosts 127.0.0.1 www1 localhost www1.home localhost4 localhost4.localdomain4 Eliezer
[RFC] deprecate hierarchy_stoplist
The hierarchy_stoplist directive has been a rarely needed and redundant directive for some time now. It's useful operation is also performed by this following config: acl stoplist url_regex cgi-bin \? always_direct allow stoplist Any objections? Amos
Re: NA - token = fatalf
On 13/02/2013 11:33 a.m., Henrik Nordström wrote:
tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov:
Hello,
Could somebody with better authentication and helper knowledge clarify
whether the token field is indeed required for Nagotiate ERR and NA
responses? If not, can we just remove the above quoted fatalf() blob and
make the following line conditional on the token presence?
Squid-2 negotiate expects
NAblobmessage
but do not require any of them to be present. It accepts
NA
as valid response.
NTLM is slightly different and do not expect a blob. Simply
NAmessage
where message may be blank.
Regards
Henrik
Squid-3 should be identical. The token is required for Authenticate-Info
to supply client with keytab identification in the reponse headers. A
missing token= on the Negotiate response normally indicates that an NTLM
helper has been wrongly configured on the Negotiate auth interface.
Markus' negotiate_wrapper helper presents a dummy token when mapping
NTLM responses to Squid.
Yes you can remove these fatal() if you want, but it needs to result in
authentication failure and squid.conf ERROR messages if you do so. The
code for triggering all that side-effect is in the BrokenHelper use case
which might need to become a separate error handling method. This also
goes for the other identical fatal("Unsupported helper response") in
Negotiate auth which would be worth removing in the same way.
Amos
Re: icap protocal error
On 11/02/2013 7:57 p.m., raja wrote: Hi, while my traffic is going through squid proxy some times browsing becomes damn slow..after some time getting icap protocol error ..after few minutes able to browse again.Let me know if anybody have idea about this issue Detected protocol errors cause the connection having errors to be closed to protect against corruption. If the ICAP server errors on many connections at once or within a short period there is a lot of work for Squid to do to open new ones and re-try the failed messages before the client responses may be handed back or new requests processed. Look at what that protocol error *is* and investigate solving that. Amos
Re: [PATCH] NA - token = fatalf
On 13/02/2013 2:37 p.m., Alex Rousskov wrote: On 02/12/2013 03:33 PM, Henrik Nordström wrote: tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov: Could somebody with better authentication and helper knowledge clarify whether the token field is indeed required for Nagotiate ERR and NA responses? If not, can we just remove the above quoted fatalf() blob and make the following line conditional on the token presence? Squid-2 negotiate expects NAblobmessage but do not require any of them to be present. Is the attached fix on the right track? It makes the "token" part of the helper response optional and, hence, removes the fatalf() message. No other changes were intended, but this trunk patch is untested. Thank you, Alex. See my other message.. Yes your patch looks reasonable design for removal of the *specific* fatal() on the ERR/NA switch case, but only because the normal handling of that case is auth failure and cleanup. The other fatal()'s on success should be removed as well and cannot ignore the token= or user= absence. ** Like Henrik said the message= portion may be empty or missing. If token is missing it is much more likely that message is also missing, otherwise on NA the first word of the message would very likely be wrongly mapped as the token. So the messageNote dereference will need to be protected as well in this change. Amos
Re: NA - token = fatalf
On 13/02/2013 3:00 p.m., Alex Rousskov wrote:
On 02/12/2013 06:41 PM, Amos Jeffries wrote:
On 13/02/2013 11:33 a.m., Henrik Nordström wrote:
tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov:
Hello,
Could somebody with better authentication and helper knowledge clarify
whether the token field is indeed required for Nagotiate ERR and NA
responses? If not, can we just remove the above quoted fatalf() blob and
make the following line conditional on the token presence?
Squid-2 negotiate expects
NAblobmessage
but do not require any of them to be present. It accepts
NA
as valid response.
NTLM is slightly different and do not expect a blob. Simply
NAmessage
where message may be blank.
Regards
Henrik
Squid-3 should be identical. The token is required for Authenticate-Info
to supply client with keytab identification in the reponse headers.
Can you clarify (for those who do not know enough about authentication)
whether the token is required only if the admin wants some optional
functionality to work? Like some optional header to be sent by Squid to
the user? Or is it required for the error page to be reliably displayed
to the authenticating user at all?
My understanding is that the admin does not want authentication to
continue in this NA/ERR case.
I'm not sure exactly what it is used for on failures, or if we need to
send it at all. Care to experiment?
From that and the snippet of RFC 4559 "
If a 401 containing a "WWW-Authenticate"
header with "Negotiate" and gssapi-data is returned from the server,
it is a continuation of the authentication request.
"
It does not make any separation between challenge responses or re-try
authenticate states (NA with a token). I am guessing that it could be
used to negotiate acceptible GSSAPI sub-protocols when validation
failure occurs. Or re-start of the authentication session if nonce sent
by the client is broken.
A
missing token= on the Negotiate response normally indicates that an NTLM
helper has been wrongly configured on the Negotiate auth interface.
Markus' negotiate_wrapper helper presents a dummy token when mapping
NTLM responses to Squid.
I do not know whether this particular helper is misconfigured. And I
cannot tell by reading the wiki page (for the reasons discussed
earlier). I believe the helper works OK otherwise. In the NA case, the
helper returns something like NA NT_STATUS_NO_SUCH_USER *\n
Is that a valid NA response?
Looks like it should be correct.
Yes ERR token="NT_STATUS_NO_SUCH_USER" message="*"
I spy the HelperReply.cc parse() "NA " case is not converting the
responses. AF is doing so, but not NA. Neither is the Negotiate switch
case. This would seem to be the main bug.
IIRC that was left because there was confusion over how to handle
multi-word messages and optional token prefix.
Questions are:
do we assume that there is no token on NA and send the whole string as
visible error message? The proposed patch will do that.
Do we try to check the first word and see if it is possibly a token and
use it as one when is found?
Or, always assume the first word is a token and send it as one? that
would probably break NTLM so we would have to do it in the Negotiate
swicth case. But would allow us to send the catenated token+message for
error display in case we got it wrong and a FUD token in Negotiate
should not hurt.
... and yes these are blind guess ideas. I have not investigated any
side effects.
If the token is required for NA/ERR responses, then the helper is broken
because it does not send one. If the token is not required, then Squid
is broken because it requires one. Henrik says the token is not required
[in Squid2].
It seems to be sending token "NT_STATUS_NO_SUCH_USER".
Henrick only said these two were valid:
NA \n
NA token message\n
Although I would go so far as to add this one is probably valid also:
NA token\n
Yes you can remove these fatal() if you want, but it needs to result in
authentication failure
Will removal of fatal(), as in the posted patch, result in
authentication failure? I assume it will, but, again, I am not an
authentication expert.
As done yes it should.
and squid.conf ERROR messages if you do so.
Do you mean squid.cache ERROR message? At level 1? Does that imply that
token is required? I doubt the admin would want tons of ERROR messages
in the cache.log so they would probably have to fix their helper
instead. But we should not force them to fix the helper if there is
nothing wrong with it.
Nix that now.
The
code for triggering all that side-effect is in the BrokenHelper use case
which might need to become a separate error handling method. This also
goes for the other identical fatal("Unsupported helper response") in
Negotiate auth which would be worth removing in the same way.
Tthis seems to imply that the helper is broken because the token
Re: squid 3.3.1 - assertion failed with dstdom_regex with IP based URL
On 13/02/2013 4:41 p.m., Amm wrote: I had reported this bug earlier in Dec 2012 but probably went unnoticed in squid-dev group http://www.squid-cache.org/mail-archive/squid-dev/201212/0099.html So just re-posting as it still exists in stable branch 3.3.1 For the record this is being tracked as http://bugs.squid-cache.org/show_bug.cgi?id=3717 I have followed up there. Amos
Re: [PATCH] Refactor mime.cc
On 12/02/2013 12:01 a.m., Kinkie wrote:
Since this is a refactor please fix the coding guidelines differences as
well. I have added some here:
* private variables suffixed with '_' (ie MimeIcon::url_, MimeIcon::icon_)
Done.
* please make MimeEntry a MEMPROXY class. Possibly also MimeIcon.
Done.
* please remove empty lines added at the end of MimeEntry class definition.
Done.
* with MimeIcon url and icon members being private you are free to update to
String from char*. Which will remove the need for all the char* management
code inside that object.
I'd prefer not. I'd aim for SBuf (post-StringNg) or std::string.
String useage should not be extended at this stage.
* in MimeIcon::created there are debugs() being wrapped which need not be.
Later on there are setHeaders in the same position.
Unwrapped.
* MimeEmtry destructor opening { needs to be on the line following the
function name.
doh! Thanks
* please add a Mime:: namespace, and make the global function static
functions inside it, or members of one of the classes.
I'd consider this a futher step, to be left as TODO (together with the
rethinking of whether we really want to use regexes here, or if we'd
better off with a suffix lookup).
+1.
Amos
Re: [PATCH] NA - token = fatalf
On 13/02/2013 3:31 p.m., Amos Jeffries wrote: On 13/02/2013 2:37 p.m., Alex Rousskov wrote: On 02/12/2013 03:33 PM, Henrik Nordström wrote: tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov: Could somebody with better authentication and helper knowledge clarify whether the token field is indeed required for Nagotiate ERR and NA responses? If not, can we just remove the above quoted fatalf() blob and make the following line conditional on the token presence? Squid-2 negotiate expects NAblobmessage but do not require any of them to be present. Is the attached fix on the right track? It makes the "token" part of the helper response optional and, hence, removes the fatalf() message. No other changes were intended, but this trunk patch is untested. Thank you, Alex. See my other message.. Yes your patch looks reasonable design for removal of the *specific* fatal() on the ERR/NA switch case, but only because the normal handling of that case is auth failure and cleanup. The other fatal()'s on success should be removed as well and cannot ignore the token= or user= absence. ** Like Henrik said the message= portion may be empty or missing. If token is missing it is much more likely that message is also missing, otherwise on NA the first word of the message would very likely be wrongly mapped as the token. So the messageNote dereference will need to be protected as well in this change. Amos Alternative applied to trunk as rev.12686 (and 12687) Amos
[PATCH] support parameters for no-cache and private
Since we now support HTTP/1.1 storage and revalidation of
Cache-Control:no-cache it is important that we at least detect the cases
where no-cache= and private= contain parameters.
AFAIK these are still rare occurances due to the historic lack of
support. So for now Squid just detects and exempts these responses from
the caching performed. The basic framework for adding future support of
these HTTP/1.1 features is made available
Dmitry: Please run this past Co-Advisor to confirm the private="..." and
no-cache="..." cases are now all "Precondition Failed".
Amos
=== modified file 'src/HttpHdrCc.cc'
--- src/HttpHdrCc.cc2012-09-04 11:58:36 +
+++ src/HttpHdrCc.cc2013-02-14 07:29:50 +
@@ -194,15 +194,38 @@
}
break;
+case CC_PRIVATE:
+if (!p) {
+// Value parameter is optional.
+setMask(type,true);
+private_.clean();
+} else if (/* p &&*/ !httpHeaderParseQuotedString(p,
(ilen-nlen-1), &private_)) {
+debugs(65, 2, "cc: invalid private= specs near '" << item <<
"'");
+clearPrivate();
+} else {
+setMask(type,true);
+}
+break;
+
+case CC_NO_CACHE:
+if (!p) {
+// On Requests, missing value parameter is expected syntax.
+// On Responses, value parameter is optional.
+setMask(type,true);
+no_cache.clean();
+} else if (/* p &&*/ !httpHeaderParseQuotedString(p,
(ilen-nlen-1), &no_cache)) {
+debugs(65, 2, "cc: invalid no-cache= specs near '" << item <<
"'");
+clearNoCache();
+} else {
+// On Requests, a value parameter is invalid syntax.
+// XXX: identify when parsing request header and dump err
message here.
+setMask(type,true);
+}
+break;
+
case CC_PUBLIC:
Public(true);
break;
-case CC_PRIVATE:
-Private(true);
-break;
-case CC_NO_CACHE:
-noCache(true);
-break;
case CC_NO_STORE:
noStore(true);
break;
=== modified file 'src/HttpHdrCc.h'
--- src/HttpHdrCc.h 2012-09-21 14:57:30 +
+++ src/HttpHdrCc.h 2013-02-14 07:26:38 +
@@ -74,15 +74,15 @@
//manipulation for Cache-Control: private header
bool hasPrivate() const {return isSet(CC_PRIVATE);}
-bool Private() const {return isSet(CC_PRIVATE);}
-void Private(bool v) {setMask(CC_PRIVATE,v);}
-void clearPrivate() {setMask(CC_PRIVATE,false);}
+const String &Private() const {return private_;}
+void Private(String &v) {setMask(CC_PRIVATE,true); private_.append(v);} //
uses append for multi-line headers
+void clearPrivate() {setMask(CC_PRIVATE,false); private_.clean();}
//manipulation for Cache-Control: no-cache header
bool hasNoCache() const {return isSet(CC_NO_CACHE);}
-bool noCache() const {return isSet(CC_NO_CACHE);}
-void noCache(bool v) {setMask(CC_NO_CACHE,v);}
-void clearNoCache() {setMask(CC_NO_CACHE,false);}
+const String &noCache() const {return no_cache;}
+void noCache(String &v) {setMask(CC_NO_CACHE,true); no_cache.append(v);}
// uses append for multi-line headers
+void clearNoCache() {setMask(CC_NO_CACHE,false); no_cache.clean();}
//manipulation for Cache-Control: no-store header
bool hasNoStore() const {return isSet(CC_NO_STORE);}
@@ -166,6 +166,9 @@
int32_t max_stale;
int32_t stale_if_error;
int32_t min_fresh;
+String private_; ///< List of headers sent as value for CC:private="...".
May be empty/undefined if the value is missing.
+String no_cache; ///< List of header sent as value for CC:no-cache="...".
May be empty/undefined if the value is missing.
+
/// low-level part of the public set method, performs no checks
_SQUID_INLINE_ void setMask(http_hdr_cc_type id, bool newval=true);
_SQUID_INLINE_ void setValue(int32_t &value, int32_t new_value,
http_hdr_cc_type hdr, bool setting=true);
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2013-02-12 11:34:35 +
+++ src/client_side_request.cc 2013-02-12 12:14:34 +
@@ -1094,7 +1094,7 @@
if (!request->flags.ignoreCc) {
if (request->cache_control) {
-if (request->cache_control->noCache())
+if (request->cache_control->hasNoCache())
no_cache=true;
// RFC 2616: treat Pragma:no-cache as if it was
Cache-Control:no-cache when Cache-Control is missing
=== modified file 'src/http.cc'
--- src/http.cc 2013-02-16 11:42:53 +
+++ src/http.cc 2013-02-17 01:41:03 +
@@ -362,6 +362,16 @@
}
// NP: request CC:no-cache only means cache READ is forbidden. STORE
is permitted.
+if (rep->cache_control->ha
Re: [PATCH] Polish the Consistency of Case Sensitivity in Configurations
On 17/02/2013 8:10 p.m., Tianyin Xu wrote: Hi, all, I proposed to make it consistent for the case sensitivity of configuration parsing couple weeks ago. I saw the replies from Alex, Reiner, and Amos. All of you agreed on this (thanks a lot!). The attached patch is for this purpose. The patch follows Alex's idea, i.e., to make all the parameter names and options case sensitive. The patch is really simple that changes "strcasecmp" to "strcmp". It mainly deal with constant configuration options (e.g., enumerative options and boolean options). For directive names, it's already consistent (case sensitive), as Amos said, the parser functions are auto-generated. I didn't change the case sensitivity of the following parameter values: - user and group names - host names - domain and realm names - ACL names - filesystem names - options in request/response/digest messages Some of them should be case insensitive, and some of them I cannot judge so I have to be conservative. If you want to make any of them case sensitive, please let me know. Let me know your comments and opinions about the patch. I'm always open. Thanks, Tianyin +1. Amos
Re: [PATCH] debug prints
On 10/08/2012 1:57 p.m., Amos Jeffries wrote:
On 9/08/2012 12:12 a.m., Alexander Komyagin wrote:
Following patch fixes nested debug() calls problem.
Since debugs() is a macros, it should not change static Debugs::level
before putting debug message to the internal stream. Otherwise we
encounter problems when debug message itself contains calls to debugs().
--- src/Debug.h2012-03-07 05:42:55.0 +0300
+++ src/Debug.h2012-08-08 14:49:20.0 +0400
@@ -106,8 +106,9 @@
/* Debug stream */
#define debugs(SECTION, LEVEL, CONTENT) \
do { \
-if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \
+if ((LEVEL) <= Debug::Levels[SECTION]) { \
Debug::getDebugOut() << CONTENT; \
+Debug::level = (LEVEL); \
Debug::finishDebug(); \
} \
} while (/*CONSTCOND*/ 0)
Looks okay. How much testing has this had?
Amos
Applied as trunk rev.12698. Sorry this took so long to go in.
Amos
Re: [PATCH] Optimization: domain agnostic pconn
On 19/11/2011 9:02 a.m., Henrik Nordström wrote: lör 2011-11-19 klockan 00:56 +1300 skrev Amos Jeffries: This removes the domain from the server pconn key. I would not recommend doing this. We started out with only IP:PORT as the key, but then had to add doamin to origin server connections due to broken servers not expecting persistent connecitons crossing domain based virtual servers. Squid using cache_peer can see a large number of wasted idle connections to their peers due to the key domain value if the peer hostname is not substituted properly. There is also a similar affect when contacting servers with virtual hosted domains. domain should never be part of a cache_peer pconn key. Don't even see a need of using the peer name there. but what you describe above sounds like bugs where it somehow is forgotten that a connection was sent to a peer. Better fix those bugs than hiding their effect. Regards Henrik Sorry for leaving this one so long. It turned out the forward.cc code was being inconsistent with its use of peer hostname and name= when pushing and popping. I adjusted the patch to send NULL when peer is used but request->GetHost() on all DIRECT traffic. It appears to work almost as well as the first version. Applied to trunk as rev.12699. Amos
Re: [PATCH] debug prints
On 19/02/2013 9:22 a.m., Alex Rousskov wrote:
On 02/18/2013 04:39 AM, Amos Jeffries wrote:
On 10/08/2012 1:57 p.m., Amos Jeffries wrote:
On 9/08/2012 12:12 a.m., Alexander Komyagin wrote:
Following patch fixes nested debug() calls problem.
Since debugs() is a macros, it should not change static Debugs::level
before putting debug message to the internal stream. Otherwise we
encounter problems when debug message itself contains calls to debugs().
--- src/Debug.h2012-03-07 05:42:55.0 +0300
+++ src/Debug.h2012-08-08 14:49:20.0 +0400
@@ -106,8 +106,9 @@
/* Debug stream */
#define debugs(SECTION, LEVEL, CONTENT) \
do { \
-if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \
+if ((LEVEL) <= Debug::Levels[SECTION]) { \
Debug::getDebugOut() << CONTENT; \
+Debug::level = (LEVEL); \
Debug::finishDebug(); \
} \
} while (/*CONSTCOND*/ 0)
Looks okay. How much testing has this had?
Amos
Applied as trunk rev.12698. Sorry this took so long to go in.
Please note that debugs() is not the only macro that changes
Debug::level before doing anything else. The old do_debug() does that as
well.
debugs() still changes Debug::sectionLevel before we do anything else so
there is inconsistency now with regard to level and sectionLevel.
Finally, this change makes current debugging level unavailable to
CONTENT. I know some CONTENT uses Debug::sectionLevel. I do not see any
CONTENT using Debug::level, so this may not be a problem today.
I cannot tell exactly what problems this change is fixing (the commit
message does not explain), so the fix may very well be worth the
side-effects, but it looks like the debugging code became messier after
this change. I wonder if the fix is worth it and whether there is a
better way to fix that problem?
Possibly. Reverting. I applied because it passed the audit conditions
months back but had not been applied by anyone yet.
Amos
Re: [PATCH] support parameters for no-cache and private
On 19/02/2013 6:48 a.m., Alex Rousskov wrote:
On 02/16/2013 06:51 PM, Amos Jeffries wrote:
Since we now support HTTP/1.1 storage and revalidation of
Cache-Control:no-cache it is important that we at least detect the cases
where no-cache= and private= contain parameters.
The patch appears to do more than just detection -- Squid starts to
ignore CC:no-cache and, in some cases, CC:private (with or without
parameters, depending on circumstances).
AFAIK these are still rare occurances due to the historic lack of
support. So for now Squid just detects and exempts these responses from
the caching performed. The basic framework for adding future support of
these HTTP/1.1 features is made available
+} else if (/* p &&*/ !httpHeaderParseQuotedString(p, (ilen-nlen-1),
&private_)) {
+debugs(65, 2, "cc: invalid private= specs near '" << item <<
"'");
+clearPrivate();
+} else {
It feels wrong to clear and ignore a private CC directive that we know
is there, even though we cannot parse its value. Should we be
conservative and treat this as private without parameters instead of
possibly violating HTTP by ignoring CC:private (e.g., if our parser is
wrong)?
Yes. This is undefined behaviour, but to be on the safe side this
defaults again to "CC:private".
Also, if this code stays, please reshape it to handle the successful
parsing case (with side-effect of setting private_) first, for clarity:
if (!p) {
private_.clean(); // last CC:private wins
setMask(true, true);
} else if (parse(private_)) {
setMask(true, true); // the above [re]sets private_
} else {
clearPrivate(); // ignore this and previous CC:private
}
Same concerns for no-cache, of course.
They are formed that way to ft in with the surrounding code. In
particular the parsing stye of the other 5 options with parameters.
The HTTP semantic meaning of these three cases is very different between
themselves but the same syntax<->meaning pattern for both private and
no-cache.
CC:private ... means *whole* response is private, MUST NOT cache.
CC:private="foo" ... means the specific listed *headers* are private,
response itself MAY be cached.
CC:private=^%%$ ... (or other breakage) means invalid options and SHOULD
be ignored.
CC:private, private="foo" ... is treated in HTTP as an explicit hack by
the sender - such that caches coping with private="foo" will obey
CC:private="foo" and older 1.0 caches will do "CC:private" handling.
This patch is supposed to pull the parameters into a string for later
re-use and treat it as CC:private (MUST NOT cache). If the admin uses
ignore-private, then we require a revalidate to ensure correct response
to later clients, which should hopefully avoid cache poisoning in a lot
of cases.
+void Private(String &v) {setMask(CC_PRIVATE,true); private_.append(v);} //
uses append for multi-line headers
Would not append() merge two header names together without delimiters if
the directive is split between two header fields? For example, would not
the private_ value become "foobar" in the example below?
Foo: 1
Cache-Control: private="foo", private="bar"
Bar: 2
Fixed.
For the future code to work, the values from multiple directives should
be concatenated using ",". However, it may be better to store them in a
list or map of some kind instead (since that future code will have to
check individual header names for a match anyway). Thus, the best
structure for this may be rather different. This doubles my concern that
we are adding code that should be essentially unused (IMO), that adds
overhead, but that may have to be fixed/rewritten further when it is used.
Yes. SBufList was the idea in future. But we are not there yet. At
present we are forced to use String by the current HttpHeader mechanisms.
Also, I think the code will mishandle this case:
Foo: 1
Cache-Control: private
Cache-Control: private="bar"
Bar: 2
because it will treat the above as the following less restrictive case:
Foo: 1
Cache-Control: private="bar"
Bar: 2
instead of treating it as
Foo: 1
Cache-Control: private
Bar: 2
Does HTTP say that [later] less restrictive CC directives overwrite
[earlier] more restrictive ones?
Yes. These two directives directly contradict each other. See above for
the semantics. The resulting behaviour is undefined in the spec, but
given that Squid treats both forms as non-cacheable I don't think it is
a problem.
+// CC:no-cache (not only if there are no parameters)
+const bool CcNoCacheNoParams = (rep->cache_control->hasNoCache() &&
rep->cache_control->noCache().undefined());
The comment says "not only if there are no p
[RFC] DNS system upgrades
A few things are on my radar that need improving in the Squid DNS components. I am proposing these as a batch, any which we can agree on will be scheduled for fixing in 3.4. 1) renaming all DNS options to begin with dns_* prefix. This will allow the cfgman documentation to collate these options together as a batch in future. Also clarify what component in Squid some of the more obscure options relate to. Also, allow for some future upgrades compacting these type of options into a single component directive / command line interpreter simplicity. Options affected that I'm aware of: append_domain ignore_unknown_nameservers hosts_file cache_dns_program ... (stripping the cache_ part.) NP: at this point I am on the fence about adding the prefix to fqdncache and ipcache options and *not* proposing a change to them. 2) adapting append_domains from a string type to a custom type This will allow us to do additional configuration validation. Such as identifying whether resolv.conf or squid.conf was used as data source. * auto-enablling dns_defnames search list /etc/resolv.conf contains two similar but different directives for labelling the local domain name. The "search" directive in particular signals DNS searching of multiple domains to the default host resolver. But Squid still requires explicit "dns_defnames on" in squid.conf to enable that behaviour. As a result we have administrators seeing a 'bad' difference between internal and dnsserver when they try upgrading to internal DNS. I propose using the resolv.conf hint to enable dns_defnames searching automatically in the absence of explicit squid.conf configuration. Administrators who do not want it are supposed to be using the alternative "domain" directive, when they use tools like host or nslookup to debug they should see consistent behaviour (diverting them away from Squid to the actual DNS issues in resolv.conf settings), and this will prevent future confusion such as appears to be happening in bug 3585. 3) removal of dnsserver and related code. IRIC the argument for keeping it previously was NetBIOS or WINS name lookup still being needed (though I am suspicious the dns_defnames issue was the actual cause of this). - NetBIOS was deprecated in 2000, to be replaced by DNS - WINS was deprecated in 2012, to be replaced by IPv6 auto-configuration / adhoc services. I am okay to see this delayed a few more squid series to give WINS longer to die, but it is time we looked again at why it is still there. Since the last round of discussion we adjusted the internal engine to fix most of the remaining issues. The above dns_defnames behaviour change is the last bit I am aware of that is needed for a seamless upgrade, short of full-blown NetBIOS support integration which is not very feasible. Amos
Re: [RFC] DNS system upgrades
On 20/02/2013 8:45 p.m., Alex Rousskov wrote: On 02/19/2013 05:08 PM, Amos Jeffries wrote: A few things are on my radar that need improving in the Squid DNS components. I am proposing these as a batch, any which we can agree on will be scheduled for fixing in 3.4. 1) renaming all DNS options to begin with dns_* prefix. This will allow the cfgman documentation to collate these options together as a batch in future. Also clarify what component in Squid some of the more obscure options relate to. Also, allow for some future upgrades compacting these type of options into a single component directive / command line interpreter simplicity. Options affected that I'm aware of: append_domain ignore_unknown_nameservers hosts_file cache_dns_program ... (stripping the cache_ part.) NP: at this point I am on the fence about adding the prefix to fqdncache and ipcache options and *not* proposing a change to them. 2) adapting append_domains from a string type to a custom type This will allow us to do additional configuration validation. Such as identifying whether resolv.conf or squid.conf was used as data source. * auto-enablling dns_defnames search list /etc/resolv.conf contains two similar but different directives for labelling the local domain name. The "search" directive in particular signals DNS searching of multiple domains to the default host resolver. But Squid still requires explicit "dns_defnames on" in squid.conf to enable that behaviour. As a result we have administrators seeing a 'bad' difference between internal and dnsserver when they try upgrading to internal DNS. I propose using the resolv.conf hint to enable dns_defnames searching automatically in the absence of explicit squid.conf configuration. By "explicit squid.conf configuration", you mean the dns_nameservers option, right? In other words, if dns_nameservers is given, the entire /etc/resolv.conf is ignored. Otherwise, both "nameserver" and "search" directives in /etc/resolv.conf are honored, correct? No I meant "dns_defnames on" is needed explicitly in squid.conf to enable DNS search behaviour the search settings loaded from resolv.conf should have meant in the first place. The default squid.conf does not honour the search part of teh setting. What you point at is another behaviour I had forgotten. 5) We should make etc/resolv.conf provide the defaultsfor dns_defnames, dns_nameservers, append_domain squid.conf options and have them override resolvconf. The way append_domain and dns_nameservers work that appears to have been the intention originally. Administrators who do not want it are supposed to be using the alternative "domain" directive, when they use tools like host or nslookup to debug they should see consistent behaviour (diverting them away from Squid to the actual DNS issues in resolv.conf settings), and this will prevent future confusion such as appears to be happening in bug 3585. 3) removal of dnsserver and related code. IRIC the argument for keeping it previously was NetBIOS or WINS name lookup still being needed (though I am suspicious the dns_defnames issue was the actual cause of this). - NetBIOS was deprecated in 2000, to be replaced by DNS - WINS was deprecated in 2012, to be replaced by IPv6 auto-configuration / adhoc services. I am okay to see this delayed a few more squid series to give WINS longer to die, but it is time we looked again at why it is still there. Since the last round of discussion we adjusted the internal engine to fix most of the remaining issues. The above dns_defnames behaviour change is the last bit I am aware of that is needed for a seamless upgrade, short of full-blown NetBIOS support integration which is not very feasible. All three items above sound good to me, but this is not my area of expertise so I hope others will chime in. The WINS-related decision may be worth discussing on squid-users as well. If we can't findany reasons against that is the next step. Henrik was quick enough to provide reasons to keep it last time without bothering many people. Amos
Re: "tproxy" problem with squid 2.7 stable with centos 5.9
On 18/02/2013 8:38 p.m., Ahmad wrote: Dear all , ive installed centos 5.9 32 bit . ive compiled the kernel with balabit "tproxy patch" i downloaded the kernel & patch from balabit site . i also patched the iptables from balabit site with verison 1.4.10. i followed the articale of how to patch the kernel & iptables from http://mattiasgeniar.be/2010/09/01/compile-a-centos-kernel-and-iptables-with-tproxy-support/ ive patched the kernel & iptables successfully , ive compiled squid 2.7stable 9 with --enable netfilter Squid-2.7 does not support multiple interception modes being compiled into the proxy at once and TPROXYv2 was not built into the Netfilter components. Remove the netfilter from your build options and leave just the --enable-linux-tproxy. Also, please try an upgrade to Squid-3.2 or later. They have *much* better TPROXY support. Amos
Re: [RFC] DNS system upgrades
On 21/02/2013 10:20 a.m., Alex Rousskov wrote: On 02/20/2013 01:46 AM, Amos Jeffries wrote: On 20/02/2013 8:45 p.m., Alex Rousskov wrote: On 02/19/2013 05:08 PM, Amos Jeffries wrote: 2) adapting append_domains from a string type to a custom type This will allow us to do additional configuration validation. Such as identifying whether resolv.conf or squid.conf was used as data source. * auto-enablling dns_defnames search list /etc/resolv.conf contains two similar but different directives for labelling the local domain name. The "search" directive in particular signals DNS searching of multiple domains to the default host resolver. But Squid still requires explicit "dns_defnames on" in squid.conf to enable that behaviour. As a result we have administrators seeing a 'bad' difference between internal and dnsserver when they try upgrading to internal DNS. I propose using the resolv.conf hint to enable dns_defnames searching automatically in the absence of explicit squid.conf configuration. By "explicit squid.conf configuration", you mean the dns_nameservers option, right? In other words, if dns_nameservers is given, the entire /etc/resolv.conf is ignored. Otherwise, both "nameserver" and "search" directives in /etc/resolv.conf are honored, correct? No I meant "dns_defnames on" is needed explicitly in squid.conf to enable DNS search behaviour the search settings loaded from resolv.conf should have meant in the first place. The default squid.conf does not honour the search part of teh setting. Will enabling dns_nameservers disable any use of /etc/resolv.conf? Yes in the crurrent Squid dns_nameservers disables loading of resolv.conf entirely. I disagree on this being a desirable order of preference, but that is outside the proposed change. I'm only proposing for now that dns_defnames directive be enabled *if* resolv.conf is loaded containing search directive and nothing is in squid.conf setting it explicitly. I think there should be an easy way for an admin to disable all /etc/resolv.conf use and rely on squid.conf settings exclusively. Use of dns_nameservers may be a good trigger for that. In other words, I do not think Squid should pick up search clues from /etc/resolv.conf when the admin explicitly configured dns_nameservers. It feels like that would lead to messy configurations where the admin will not know for sure where the information is coming from. We should warn when options contradict each other. If there is a conflict, I think our preference should be towards "works as expected" rather than "external DNS works as internal DNS (and so it is easy to switch from former to the latter)". ... which to me means Squid always loading resolv.conf first and obeying its instructions, then overriding particular aspects of that behaviour with squid.conf settings. The current expectation is that with a default squid.conf the command line "host foo" and "squidclient http://foo/"; will both respond with something indicating 127.0.0.1 as the server. As of today the squidclient response is "Domain Not Found". Amos
Re: [PATCH] Preserve bare backslashes in AF and TT
On 21/02/2013 2:47 p.m., Alex Rousskov wrote:
Hello,
It looks like NTLM and possibly Negotiate authentication is broken
in trunk because Squid eats the bare backslash that AF responses use to
separate authentication domain and user names. With the backslash
removed, the merged domainuser name never matches, of course.
The attached patch fixes the problem in my tests. Is there a better way
to do this?
Thank you,
Alex.
strwordtok() is a function of our own creation so it is probably best
long term to adjust its API to include a flag for skipping the
slashDecode behaviour instead of creating duplicate code.
Example patch attached. Re-formatting is avoided to show clearly the
small amount of change needed.
I like the extra documentation you added about statics and use cases.
Can you write something similar about strwordtok() ?
Amos
=== modified file 'src/SquidString.h'
--- src/SquidString.h 2012-09-22 10:56:48 +
+++ src/SquidString.h 2013-02-21 02:21:23 +
@@ -184,6 +184,11 @@
const char *checkNullString(const char *p);
int stringHasWhitespace(const char *);
int stringHasCntl(const char *);
-char *strwordtok(char *buf, char **t);
+
+/**
+ * Like strtok(start, " ") but contains a rudimentary knowledge of quoted
strings and backslashing.
+ * When slashDecode parameter is set to false does NOT decode backslashes so
is safe for bare backslashes.
+ */
+char *strwordtok(char *buf, char **t, bool slashDecode = true);
#endif /* SQUID_STRING_H */
=== modified file 'src/String.cc'
--- src/String.cc 2012-10-04 09:14:06 +
+++ src/String.cc 2013-02-21 02:16:46 +
@@ -350,7 +350,7 @@
* of quoting
*/
char *
-strwordtok(char *buf, char **t)
+strwordtok(char *buf, char **t, bool slashDecode)
{
unsigned char *word = NULL;
unsigned char *p = (unsigned char *) buf;
@@ -376,6 +376,7 @@
switch (ch) {
case '\\':
+if (slashDecode) {
++p;
switch (*p) {
@@ -396,6 +397,7 @@
break;
}
+}
*d = ch;
++d;
Re: [RFC] DNS system upgrades
On 21/02/2013 6:15 p.m., Alex Rousskov wrote: On 02/20/2013 06:56 PM, Amos Jeffries wrote: I'm only proposing for now that dns_defnames directive be enabled *if* resolv.conf is loaded containing search directive and nothing is in squid.conf setting it explicitly. Yes, that would make sense to me: If the admin wants to use resolv.conf, we should use all of it by default. I think there should be an easy way for an admin to disable all /etc/resolv.conf use and rely on squid.conf settings exclusively. Use of dns_nameservers may be a good trigger for that. In other words, I do not think Squid should pick up search clues from /etc/resolv.conf when the admin explicitly configured dns_nameservers. It feels like that would lead to messy configurations where the admin will not know for sure where the information is coming from. We should warn when options contradict each other. If there is a conflict, I think our preference should be towards "works as expected" rather than "external DNS works as internal DNS (and so it is easy to switch from former to the latter)". ... which to me means Squid always loading resolv.conf first and obeying its instructions, then overriding particular aspects of that behaviour with squid.conf settings. I see your point. However, it may be better to simplify expectations when admin starts tweaking things by using an "either resolv.conf or squid.conf" principle. It would be unusual and unnatural, IMHO, to specify domain names manually in squid.conf but then rely on resolv.conf for "search" patterns so I am guessing most admins would not expect that kind of combination. But we do the opposite on all other directives... * "nameserver" loaded from resolv.conf and search path overridden by append_domain. * "domain" loaded from resolv.conf and overridden by dns_defnames. One possible solution to clarify choices and minimize complex dependencies is to group these options. The admin would have to pick one of the following two approaches: # either use these resolvers, with these options: dns_resolution \ resolvers=ip1,ip2,ip3 \ search=tld1,tld2 \ ... # or use resolv.conf, possibly overwriting some of its settings: dns_resolution \ config=/etc/resolv.conf \ search=... \ ... with the following being the default (no overwriting): dns_resolution \ config=/etc/resolv.conf I may be missing some details here, but I hope the above illustrates the overall approach. I understand what you mean but am not understanding why it is a good thing rather than simply documenting our directives individually as: "Squid loads the operating system DNS settings from /etc/resolv.conf. This directive overrides and replaces the XX feature of resolv.conf" Nothing complex or unexpected about that behaviour. It is already true for all the other directives when dns_nameservers is omitted (which is default). The only complexity is due to dns_nameservers disabling the entirety of resolv.conf loading. Amos
Re: [PATCH] Preserve bare backslashes in AF and TT
On 21/02/2013 6:41 p.m., Alex Rousskov wrote: On 02/20/2013 07:26 PM, Amos Jeffries wrote: On 21/02/2013 2:47 p.m., Alex Rousskov wrote: Hello, It looks like NTLM and possibly Negotiate authentication is broken in trunk because Squid eats the bare backslash that AF responses use to separate authentication domain and user names. With the backslash removed, the merged domainuser name never matches, of course. The attached patch fixes the problem in my tests. Is there a better way to do this? strwordtok() is a function of our own creation so it is probably best long term to adjust its API to include a flag for skipping the slashDecode behaviour instead of creating duplicate code. Please note that the function I added does not duplicate strwordtok() behavior. If anything, it duplicates strtok(), but for good reasons. Noted. When two functions cover very different use cases and have rather different semantics (not to mention implementation), I think they both deserve to live. Example patch attached. Re-formatting is avoided to show clearly the small amount of change needed. Are we sure that valid user names cannot contain quotes or other symbols that strwordtok() may want to process specially? Today and tomorrow? It seems reasonable to have two tokenizing APIs, one that handles shell-like escaping/quoting (and possibly variable substitution in the future) and another that just extracts words based on whitespace. Their use cases are very different. If you disagree that two functions are better than one here, how about applying the "slashDecode" flag that you want to add to _all_ special strwordtok() powers, naming it simply "decode"? After all, if backslashes are not treated specially, not everything can be successfully quoted anyway. In other words, if input format does not support an escape mechanism such as backslashes it cannot fully support quotes either. I do disagree that two functions are needed and am happy with a flag disabling all the special handling in strwordtok(). I like the extra documentation you added about statics and use cases. Can you write something similar about strwordtok() ? Sure, will do (regardless of whether we add a second tokenizing function or not). Thank you. Amos
Re: [PATCH] support parameters for no-cache and private
Adjusted patch to drop the odd NP, rework CC:private operation on broken
parameters, and fix the segfault.
Since we now support HTTP/1.1 storage and revalidation of
Cache-Control:no-cache it is important that we at least detect the
cases where no-cache= and private= contain parameters and handle them
properly whenever possible.
AFAIK these are still rare occurances due to the historic lack of
support. So for now Squid just detects and exempts these responses
from the caching performed. The basic framework for adding future
support of these HTTP/1.1 features is made available
Please run this past Co-Advisor to confirm the private="..." and
no-cache="..." cases are now all "Precondition Failed".
Amos
=== modified file 'src/HttpHdrCc.cc'
--- src/HttpHdrCc.cc2012-09-04 11:58:36 +
+++ src/HttpHdrCc.cc2013-02-19 02:52:30 +
@@ -194,15 +194,37 @@
}
break;
+case CC_PRIVATE:
+if (!p) {
+// Value parameter is optional.
+private_.clean();
+} else if (/* p &&*/ !httpHeaderParseQuotedString(p,
(ilen-nlen-1), &private_)) {
+debugs(65, 2, "cc: invalid private= specs near '" << item <<
"'");
+clearPrivate();
+}
+// to be safe we ignore broken parameters, but always remember the
'private' part.
+setMask(type,true);
+break;
+
+case CC_NO_CACHE:
+if (!p) {
+// On Requests, missing value parameter is expected syntax.
+// On Responses, value parameter is optional.
+setMask(type,true);
+no_cache.clean();
+} else if (/* p &&*/ httpHeaderParseQuotedString(p, (ilen-nlen-1),
&no_cache)) {
+// On Requests, a value parameter is invalid syntax.
+// XXX: identify when parsing request header and dump err
message here.
+setMask(type,true);
+} else {
+debugs(65, 2, "cc: invalid no-cache= specs near '" << item <<
"'");
+clearNoCache();
+}
+break;
+
case CC_PUBLIC:
Public(true);
break;
-case CC_PRIVATE:
-Private(true);
-break;
-case CC_NO_CACHE:
-noCache(true);
-break;
case CC_NO_STORE:
noStore(true);
break;
=== modified file 'src/HttpHdrCc.h'
--- src/HttpHdrCc.h 2012-09-21 14:57:30 +
+++ src/HttpHdrCc.h 2013-02-19 03:07:16 +
@@ -74,15 +74,27 @@
//manipulation for Cache-Control: private header
bool hasPrivate() const {return isSet(CC_PRIVATE);}
-bool Private() const {return isSet(CC_PRIVATE);}
-void Private(bool v) {setMask(CC_PRIVATE,v);}
-void clearPrivate() {setMask(CC_PRIVATE,false);}
+const String &Private() const {return private_;}
+void Private(String &v) {
+setMask(CC_PRIVATE,true);
+// uses append for multi-line headers
+if (private_.defined())
+private_.append(",");
+private_.append(v);
+}
+void clearPrivate() {setMask(CC_PRIVATE,false); private_.clean();}
//manipulation for Cache-Control: no-cache header
bool hasNoCache() const {return isSet(CC_NO_CACHE);}
-bool noCache() const {return isSet(CC_NO_CACHE);}
-void noCache(bool v) {setMask(CC_NO_CACHE,v);}
-void clearNoCache() {setMask(CC_NO_CACHE,false);}
+const String &noCache() const {return no_cache;}
+void noCache(String &v) {
+setMask(CC_NO_CACHE,true);
+// uses append for multi-line headers
+if (no_cache.defined())
+no_cache.append(",");
+no_cache.append(v);
+}
+void clearNoCache() {setMask(CC_NO_CACHE,false); no_cache.clean();}
//manipulation for Cache-Control: no-store header
bool hasNoStore() const {return isSet(CC_NO_STORE);}
@@ -166,6 +178,9 @@
int32_t max_stale;
int32_t stale_if_error;
int32_t min_fresh;
+String private_; ///< List of headers sent as value for CC:private="...".
May be empty/undefined if the value is missing.
+String no_cache; ///< List of header sent as value for CC:no-cache="...".
May be empty/undefined if the value is missing.
+
/// low-level part of the public set method, performs no checks
_SQUID_INLINE_ void setMask(http_hdr_cc_type id, bool newval=true);
_SQUID_INLINE_ void setValue(int32_t &value, int32_t new_value,
http_hdr_cc_type hdr, bool setting=true);
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2013-02-12 11:34:35 +
+++ src/client_side_request.cc 2013-02-12 12:14:34 +
@@ -1094,7 +1094,7 @@
if (!request->flags.ignoreCc) {
if (request->cache_control) {
-if (request->cache_control->noCache())
+if (request->cache_control->hasNoCache())
no_cache=true;
Re: [PATCH] support parameters for no-cache and private
On 23/02/2013 10:20 a.m., Alex Rousskov wrote:
On 02/21/2013 06:26 PM, Amos Jeffries wrote:
Adjusted patch to drop the odd NP, rework CC:private operation on broken
parameters, and fix the segfault.
Hi Amos,
Thank you for addressing most of my concerns.
+} else if (/* p &&*/ !httpHeaderParseQuotedString(p, (ilen-nlen-1),
&private_)) {
+debugs(65, 2, "cc: invalid private= specs near '" << item <<
"'");
+clearPrivate();
+}
This still does not work correctly when there are multiple valid
CC:private=".." header fields in a message header because the
httpHeaderParseQuotedString() clears the previous private_ contents, right?
You can fix this by parsing into a local String variable and then
appending that variable contents to the previously stored headers using
the now-fixed Private() method. This approach will also eliminate the
need to call clearPrivate() on failures and then immediately undo that
with setMask(true,true).
Same concern for no-cache="..." parsing.
Hmm. Right and these two are different from the other options in that
the others are singleton options. Will do.
+} else {
+debugs(65, 2, "cc: invalid no-cache= specs near '" << item <<
"'");
+clearNoCache();
+}
I still do not think we should ignore the whole CC:no-cache just because
we cannot parse something, especially when there was a perfectly valid
parameterless CC:no-cache before we failed to parse something. Instead,
we should conservatively treat the whole thing as CC:no-cache.
In the no-cache cases the recovery action is not critical like private
might be. All we risk with this is a cache HIT vs REFRESH.
Still I'm not partial either way. Will do.
In other words,
Cache-control: no-cache
Foo: bar
Cache-control: no-cache="foo
should be treated as
Cache-control: no-cache
Foo: bar
rather than
Foo: bar
You obviously disagree. Hopefully somebody will break this tie.
All these permutations have been worrying me a little.
// NP: request CC:no-cache only means cache READ is forbidden. STORE
is permitted.
+if (rep->cache_control && rep->cache_control->hasNoCache() &&
rep->cache_control->noCache().defined()) {
+/* TODO: we are allowed to cache when no-cache= has parameters.
+ * Provided we strip away any of the listed headers unless they
are revalidated
+ * successfully (ie, must revalidate AND these headers are
prohibited on stale replies).
+ * That is a bit tricky for squid right now so we avoid caching
entirely.
+ */
+debugs(22, 3, HERE << "NO because server reply Cache-Control:no-cache
has parameters");
+return 0;
+}
+
// NP: request CC:private is undefined. We ignore.
// NP: other request CC flags are limiters on HIT/MISS. We don't care
about here.
Please move the new code below all three NPs. As rendered now, the first
NP seems to apply to the if-statement, which confused me a lot. It is
also good to keep request CC code/comments together so that "other
request CC flags" comment makes more sense.
On a separate note, the code became more confusing (IMO) because patched
Squid now handles CC:no-cache in two places: Here, we handle the
parameter case. The caller handles both parameter and parameterless
cases by setting ENTRY_REVALIDATE. Fixing that is difficult, but we
should at least clarify what is going on. I suggest using the following
comment instead of the above TODO:
/* We are allowed to store CC:no-cache. When CC:no-cache has no
parameters, we must revalidate. The caller does that by setting
ENTRY_REVALIDATE. TODO: When CC:no-cache has parameters, strip away the
listed headers instead of revalidating (or when revalidation fails). For
now, avoid caching entirely because we do not strip on failed
revalidations. */
This will point to the other critical piece of code AND explain why
parameter case is treated "worse" than the parameterless one despite
being "better" from protocol intent point of view.
If my "because we do not strip on failed revalidations" explanation is
not correct, please let me know.
Okay.
Since we now support HTTP/1.1 storage and revalidation of
Cache-Control:no-cache it is important that we at least detect the
cases where no-cache= and private= contain parameters and handle them
properly whenever possible.
AFAIK these are still rare occurances due to the historic lack of
support. So for now Squid just detects and exempts these responses
from the caching performed. The basic framework for adding future
support of these HTTP/1.1 features is made available
Please run this past Co-Advisor t
[RFC] OpenSSL capability detection
As you may be aware OpenSSL had some API changes which we dutifully wrote #if-#else conditional code for using the mechanisms provided by OpenSSL for the purpose. Then somebody in Fedora or RHEL decided to back-port the functionality into their older OpenSSL version. This corrupted the Fedora 17 release for a short while, just long enough to corrupt the main RHEL 5.* and 6.* distributions, and it now seems to have spread into the CentOS 6.* distributions as well. We are urgently needing somebody to write a ./configure-time test to detect these old corrupted OpenSSL packages which present 1.0.0d API functionality and AC_DEFINE a macro which can be added to the #if-#else conditionals such that they are built using the 1.0.0d+ API of OpenSSL. Any takers? Amos
Squid SMP on MacOS
I'm trying to get the MacOS builds of Squid going again but having some problems with shm_open() in the Rock storage unit-tests. 1) MacOS defines the max name length we can pass to shm_open() at 30 bytes. "/squid-testRock__testRockSearch" being 35 or so bytes. Cutting the definition in testRock.cc down so it becomes "/squid-testRock_Search" resolves that, but then we hit (2). 2) With the short string above and the current settings sent to shm_open() in src/ipc/mem/Segment.cc line 73 MacOS shm_open() starts responding with EINVAL. Any ideas? Amos
