Re: [PATCH] validate -n parameter value
LGTM, only: Are the first two columns really necessary in the statement ::Parser::Tokenizer tok ? On Sun, Jun 15, 2014 at 8:05 AM, Amos Jeffries squ...@treenet.co.nz wrote: Update the service_name global to an SBuf and use Tokenizer to validate the -n argument is a pure alphanumeric. Amos -- Francesco
Re: [PATCH] validate -n parameter value
On 16/06/2014 9:32 p.m., Kinkie wrote: LGTM, only: Are the first two columns really necessary in the statement ::Parser::Tokenizer tok ? Maybe not in this location. But its required in this global namespace location other namespaces I've been using it there was clashes with component-secific Parser classes. So I've been keeping consistency. Amos
Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID
On 06/15/2014 12:07 AM, Amos Jeffries wrote: On 15/06/2014 4:58 a.m., Alex Rousskov wrote: On 06/11/2014 08:52 AM, Tsantilas Christos wrote: I must also note that this patch adds an inconsistency. All annotation key=values pairs received from helpers, accumulated to the existing key notes values. The clt_conn_id=Id pair is always unique and replaces the existing clt_conn_id=Id annotation pair. We may want to make all annotations unique, or maybe implement a configuration mechanism to define which annotations are overwriting their previous values and which appending the new values. I suggest making all annotations unique (i.e., new values overwrite old ones) because helpers that want to accumulate annotation values can do that by returning old values along with new ones: received by helper: name=v1 returned by helper: name=v1,v2 Please note that the opposite does not work: If annotation values are always accumulated, a helper cannot overwrite/remove the old value. Doing that would mean passing all existing annotations to every helper lookup. Why would that mean the above? AFAICT, the helper should get only the annotations it needs. That need is helper-specific and, hence, is configurable via the various _extras and equivalent directives. That is already supported and does not need to change. Here is the overall sketch for supporting unique annotations: 1. Send the helper the annotations it is configured to get (no changes here). 2. For each unique annotation key received from the helper, remove any old annotation(s) with the same key. 3. Store annotations received from the helper (no changes here). To support explicit annotation deletion, we can adjust #3 to skip key-value pairs with the value equal to '-'. HTH, Alex.
Re: [PATCH] validate -n parameter value
On 06/15/2014 12:05 AM, Amos Jeffries wrote:
+if (optarg) {
+SBuf t(optarg);
+::Parser::Tokenizer tok(t);
Pleaser remove t as used-once and lacking a descriptive name. If you
insist on keeping it, please make it const and try to give it a more
descriptive name such as rawName or serviceName.
+const CharacterSet chr =
CharacterSet::ALPHA+CharacterSet::DIGIT;
Is there a document stating that only those characters are valid? My
quick search resulted in [1] that seems to imply many other characters
are accepted. However, [2] lists more prohibited characters. Both seem
to allow - and _ though. It would be good to provide a reference in
the code or commit message to explain why we are restricting its value.
Do you want to validate the total service name length? [1] says the
limit is 256 characters.
None of the above applies to -n used on Unix. Do we need to validate the
service name (whatever that is) there?
+if (!tok.prefix(service_name, chr) || !tok.atEnd())
Please note that the above also rejects empty service names (-n ).
That may be good, but the error messages do not reflect that restriction.
+fatal(Need to provide alphanumeric service name when
using -n option);
+opt_signal_service = true;
+} else {
+fatal(Need to provide alphanumeric service name when using
-n option);
+}
I recommend adjusting the fatal() messages to distinguish the two errors
and avoid a possible misunderstanding that the service name was provided
but was not alphanumeric in the second case:
* Expected alphanumeric service name for the -n option but got: ...
* A service name is required for the -n option.
I continue to dislike the undefined -n option meaning on non-Windows
boxes, but I have no objection to this patch and believe the above
changes can be done without a new review cycle.
Cheers,
Alex.
[1] http://msdn.microsoft.com/en-us/library/ms682450%28VS.85%29.aspx
[2] http://www.anzio.com/resources/cannot-install-or-start-windows-service
Re: [code] [for discussion] map-trie
On 14 Jun 2014, at 18:34, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/14/2014 12:39 AM, Kinkie wrote: On Fri, Jun 13, 2014 at 4:50 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 06/11/2014 04:43 PM, Kinkie wrote: level-compact-trie: the mean time is 11 sec; all runs take between 10.782 and 11.354 secs; 18 Mb of core used full-trie: mean is 7.5 secs +- 0.2secs; 85 Mb of core. splay-based: mean time is 16.3sec; all runs take between 16.193 and 16.427 secs; 14 Mb of core How about std::map? Have you considered using a standard class for this purpose? std::map doesn't offer efficient prefix matching, but sure, I'll try to prepare a test run on the same data to establish a baseline. Is explicit support to prefix matching actually required though? I am thinking of using std::map::lower_bound or similar to find the vicinity of a possible prefix match. For example, when the map has a b.c domain stored, and you are searching for a.b.c, the lower_bound method returns a pointer to the stored b.c node. After that, a simple prefix test between the two strings can return the final match/mismatch answer. Needless to say, all domain names ought to be stored/compared in the reverse order of their labels. For example, a.b.c is stored internally as c.b.a. It just feels more natural for me to render it as a.b.c in a human-oriented text like this email. And if we need to support prefixes that do not end at domain label boundaries (i.e., *foo.bar should match zzzfoo.bar), then we just treat each character as a label. More data: I haven't yet tested out your solution, but yet another round of TrieNode (it's easier), which uses a variable-length std::vector + an offset to compact the TrieNode to the actual range that's needed for that one node. The results are very interesting: This implementation uses the least memory of the trie-based solutes (16.5Mb for the test data set) and performance is only 12% worse than full-fledged trie (8.5sec +- 0.3sec over 10 tests). More to come. Kinkie
Jenkins build is back to normal : 3.HEAD-amd64-FreeBSD-10-clang #97
See http://build.squid-cache.org/job/3.HEAD-amd64-FreeBSD-10-clang/97/changes
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 06/15/2014 12:02 AM, Amos Jeffries wrote: On 15/06/2014 4:44 a.m., Alex Rousskov wrote: On 06/13/2014 07:25 PM, Amos Jeffries wrote: On 14/06/2014 8:07 a.m., Alex Rousskov wrote: On 04/25/2014 02:59 AM, Amos Jeffries wrote: On 25/04/2014 12:55 p.m., Alex Rousskov wrote: Do not leak [SSL] objects tied to http_port and https_port on reconfigure. PortCfg objects were not destroyed at all (no delete call) and were incorrectly stored (excessive cbdata locking). This change adds destruction and removes excessive locking to allow the destructed object to be freed. It also cleans up forgotten(?) clientca and crlfile PortCfg members. This change fixes a serious leak but also carries an elevated risk: There is a lot of code throughout Squid that does not check the pointers to the objects that are now properly destroyed. It is possible that some of that code will crash some time after reconfigure. It is not possible to ensure that this does not happen without rewriting/fixing the offending code to use refcounting. Such a rewrite would be a relatively large change outside this patch scope. We may decide that it is better to leak than to take this additional risk. -0. I have a patch moving the SSL config options into a standalone ref-counted object. That can be polished up and references added to each ConnStateData fairly easily. Amos, what is the status of that patch? Any ETA? Stalled behind the larger works. If it is urgent I can did it out and polish it up. [...] The design is a new Ref-Countable class to hold all the SSL options (and generated state) leaving just a Pointer to it in the main config class. * Ports which needed a clone operation took a copy of the pointer and share the context. * client/server context initialization functions take a Pointer to the class and update its state content. Why treat SSL options differently from all other port options? Should not the whole PortCfg object be refcounted instead? The other options are POD-style options. They can be reset relatively okay by the parsers memset() and are filled out by the config parse routines. AFAICT, not all other options are POD-style options that can be reset relatively okay by memset() unless relatively OK means small leaks are OK. Some non-SSL port options require specialized cleanup just like some SSL port options do (PortCfg::name and defaultsite are two examples with listenConn arguably being the third). Thus, the requirement for specialized cleanup is not a reason to treat SSL options differently from all other port options. Also, the same set of config settings are needed by *_port, cache_peer, and sslproxy_* directives. It makes sense to have one class type for SSL config which is used by all three. While it may indeed be a good idea to group related options together, such grouping is irrelevant for the purpose of this discussion. Whether we group SSL-related options or not, they and other port options will continue to leak. They leak not because they are not grouped but because Squid leaks the Port structure they belong to (grouped or not). On the other side AnyP::PortCfgPointer already exists to reference count the port objects. We could spend time converting the raw pointers to use it instead so your patch here is not dangerous. PortCfgPointer is not a reference counting pointer. It is a cbdata-protected pointer. The proposed patch _is_ using/adding the associated cdbdata protections in/to some users (but more work may be needed to make all users safe, as disclosed). Simply converting all raw port pointers to use PortCfgPointer is not going to make us much safer (it will just make triaging crashes easier). The callers need to be changed to check that the pointer is still valid before dereferencing it. Unlike refcounting pointers, cbdata pointers do not protect naive code from crashing. The cbdata interface is also asymmetric -- there supposed to be only one object owner. Cbdata pointers are like std::weak_ptr and refcounting pointers are like std::shared_pointer. Very different APIs, offering different trade-offs. Most likely, we should use the refcounting API for port pointers. Until that (or a better) solution is implemented, we should either * stop port leaks or * acknowledge that we would rather see Squid leaking than increasing the probability of a Squid crash after reconfigure. Both approaches are reasonable. The proposed patch does the former. I do not want to commit that patch because there is only one vote and it is -0. What bothers me here is that the justification for the -0 vote was (AFAICT) a promise of a better patch, but it now appears that the better patch is not coming soon, has problems of its own, and may not even remove the leak. I cannot really combat those obstacles, and it is difficult for me to explain/justify them to others. May I re-interpret your -0 vote justification as the proposed fix is too risky? Thank you, Alex.
Jenkins build is back to normal : 3.HEAD-amd64-FreeBSD-9.1-clang #595
See http://build.squid-cache.org/job/3.HEAD-amd64-FreeBSD-9.1-clang/595/changes
Re: [PATCH 1/8] reconfiguration leaks: implicit ACLs
On 06/14/2014 11:12 AM, Kinkie wrote: I agree on the objective, and while this is not the solution it's a a workaround; +1 but if you haven't already please add a TODO mentioning the eventual refcount objective. Committed as trunk r13469, with an explicit TODO. Thank you, Alex. On Sat, Jun 14, 2014 at 3:21 AM, Amos Jeffries squ...@treenet.co.nz wrote: On 14/06/2014 7:57 a.m., Alex Rousskov wrote: On 04/25/2014 01:58 AM, Amos Jeffries wrote: On 25/04/2014 12:46 p.m., Alex Rousskov wrote: Do not leak implicit ACLs during reconfigure. Many ACLs implicitly created by Squid when grouping multiple ACLs were not destroyed because those implicit ACLs where not covered by the global ACL registry used for ACL destruction. See also: r13210 which did not go far enough because it incorrectly assumed that all InnerNode() children are aclRegister()ed and, hence, will be centrally freed. -0. Is this a negative vote from Squid3 voting rules point of view? http://wiki.squid-cache.org/MergeProcedure#Squid3_Voting It is I don't like it but not objecting to a commit if you do it. I believe we should move to reference counting ACLs instead of continuing to work around these edge cases. I agree that reference counting is an overall better design for ACLs, of course. However, since refcounting ACLs would be a large change that nobody has volunteered to implement in the foreseeable future (AFAIK), I suggest that this [significant] leak fix should go in now. Any other votes/opinions? Thank you, Alex.
Jenkins build is back to normal : 3.HEAD-amd64-FreeBSD-7.2 #2216
See http://build.squid-cache.org/job/3.HEAD-amd64-FreeBSD-7.2/2216/changes
