Re: [PATCH] Updates to qos_flows documentation
On Fri, 2014-03-28 at 10:48 +, Andrew Beverley wrote: > Minor updates to the qos_flows documentation. Clearer instructions > based on recent squid-users mailing list questions. Apologies, please find attached updated patch. I have made clearer the requirement to use CONNMARK not MARK. Andy Minor updates to the qos_flows documentation. Clearer instructions based on recent squid-users mailing list questions. === modified file 'src/cf.data.pre' --- src/cf.data.pre 2014-03-23 02:56:12 + +++ src/cf.data.pre 2014-03-28 11:12:17 + @@ -2071,10 +2071,19 @@ LOC: Ip::Qos::TheConfig DOC_START Allows you to select a TOS/DSCP value to mark outgoing - connections with, based on where the reply was sourced. For - platforms using netfilter, allows you to set a netfilter mark + connections to the client with, based on where the reply was sourced. + For platforms using netfilter, allows you to set a netfilter mark value instead of, or in addition to, a TOS value. + By default this functionality is disabled. To enable it with the default + settings simply use "qos_flows mark" or "qos_flows tos". Default + settings will result in the netfilter mark or TOS value being copied + from the upstream connection to the client. Note that it is the connection + CONNMARK value not the packet MARK value that is copied. + + It is not currently possible to copy the mark or TOS value from the + client to the upstream connection request. + TOS values really only have local significance - so you should know what you're specifying. For more information, see RFC2474, RFC2475, and RFC3260.
[PATCH] Updates to qos_flows documentation
Hi guys, Please find attached patch for minor updates to the qos_flows documentation, based on recent squid-users questions. Andy Minor updates to the qos_flows documentation. Clearer instructions based on recent squid-users mailing list questions. === modified file 'src/cf.data.pre' --- src/cf.data.pre 2014-03-23 02:56:12 + +++ src/cf.data.pre 2014-03-28 10:44:07 + @@ -2071,10 +2071,18 @@ LOC: Ip::Qos::TheConfig DOC_START Allows you to select a TOS/DSCP value to mark outgoing - connections with, based on where the reply was sourced. For - platforms using netfilter, allows you to set a netfilter mark + connections to the client with, based on where the reply was sourced. + For platforms using netfilter, allows you to set a netfilter mark value instead of, or in addition to, a TOS value. + By default this functionality is disabled. To enable it with the default + settings simply use "qos_flows mark" or "qos_flows tos". Default + settings will result in the mark or TOS value being copied from the + upstream connection to the client. + + It is not currently possible to copy the mark or TOS value from the + client to the upstream connection request. + TOS values really only have local significance - so you should know what you're specifying. For more information, see RFC2474, RFC2475, and RFC3260.
Re: [RFC] Package download pages
On Mon, 2012-01-23 at 11:42 +1300, Amos Jeffries wrote: > The current website layout for Versions/ and Download/ is a little > confusing and I would like to combine the two into a simpler form. ... > Comments? Other ideas? > +1 from me. I have hunted around several times for the link to the required download I want! Andy
Re: Reading ACL configuration files every request
On Mon, 2011-11-07 at 11:59 +1300, Amos Jeffries wrote: > Well, in light of the facts that new helpers are only being added to > 3.3 now That's fair enough. I've attached my (fairly raw) helper to this email anyway, just for the list archives, in case anyone else has use for it. > and that live re-configuration via POST to the manager is very > close now I'm not sure how much use this would be. Sounds good. Look forward to it! > > On a similar subject, is there any mileage in making the FORMAT > > optional > > for external_acl_type? There is obviously no need for it in this > > case, > > although as you have shown it is easy to workaround with a fairly > > static > > parameter. > > The long term plans are to make the external ACL format merge with log > line format codes and add a format= option. Allowing far more > flexibility in the format syntax. Great news. > I've just added support for the '%%' token which can be used for a > completely static placeholder. Thanks. Andy datetime_acl.pl Description: Perl program
Re: Reading ACL configuration files every request
On Mon, 2011-11-07 at 13:55 -0700, Alex Rousskov wrote: > FWIW, the usual approach (in a helper or elsewhere) is to reread the > configuration file only when the file modification time stamp changes > and/or a HUP signal is received, and not on every request. Thanks, I hadn't thought of that. Now incorporated into my helper! Andy
Re: Reading ACL configuration files every request
On Sun, 2011-11-06 at 14:17 +1300, Amos Jeffries wrote: > On 6/11/2011 1:39 p.m., Andrew Beverley wrote: > > Hi, > > > > I am using the ACL feature whereby the parameters can be read from a > > file. For example: > > > > acl session_day time "/var/www/announce_days.txt" > > > > Understandably, the file only appears to be read when the configuration > > file is parsed, rather than each time the ACL is checked. However, I > > need it to be checked more often, as I have a system configuration > > interface that writes a day of the week to the file, which subsequently > > causes a splash page to be shown on a particular day. I would like > > configuration to be done without having to reload the Squid > > configuration file. > > > > Would any consideration be given to a patch to check the ACL file more > > often? Could/should it be an extra configuration option to check the > > file each request? I appreciate that this would come with a greater > > overhead. Is there a better way to achieve this? > > A better way currently available would be to use an external_acl helper > to read and response OK/FAIL. You probably want to pass something > relatively static (%PROTO or %METHOD) as the ACL format to reduce the > overheads. With time calculations vs "now" you can ignore the actual > input format. Good idea, thanks. I've now written a little perl helper to do this. It would be nice to add this to the main squid source code. Shall I just submit as a bzr diff as normal? Is there any documentation for the external helpers that I should also add to? On a similar subject, is there any mileage in making the FORMAT optional for external_acl_type? There is obviously no need for it in this case, although as you have shown it is easy to workaround with a fairly static parameter. Andy
Reading ACL configuration files every request
Hi, I am using the ACL feature whereby the parameters can be read from a file. For example: acl session_day time "/var/www/announce_days.txt" Understandably, the file only appears to be read when the configuration file is parsed, rather than each time the ACL is checked. However, I need it to be checked more often, as I have a system configuration interface that writes a day of the week to the file, which subsequently causes a splash page to be shown on a particular day. I would like configuration to be done without having to reload the Squid configuration file. Would any consideration be given to a patch to check the ACL file more often? Could/should it be an extra configuration option to check the file each request? I appreciate that this would come with a greater overhead. Is there a better way to achieve this? Thanks, Andy
Squid workers segfault occasionally - v3.2.0.13
Hi, I'm getting occasionally Squid segfaults (from the worker I think) with version 3.2.0.13. During busy periods these can be several times an hour, at other times it could be once or twice a day. What is the best way of tracing the problem? Should I increase the logging verbosity and see if I can get some useful information? I have copied the logs that I am seeing in the system log below, but appreciate that these may not be of much help: Nov 3 18:28:37 nelsonwr kernel: [39335.587192] squid3[15190]: segfault at 0 ip 0055f9b0 sp 7fffc5674ed0 error 4 in squid[40+3be000] Nov 3 19:50:42 nelsonwr kernel: [44260.802547] squid3[17752]: segfault at 0 ip 0055f9b0 sp 7fff1145b1e0 error 4 in squid[40+3be000] Nov 3 20:07:37 nelsonwr kernel: [45275.771624] squid3[21493]: segfault at 0 ip 0055f9b0 sp 7fffd8413160 error 4 in squid[40+3be000] Nov 3 20:34:08 nelsonwr kernel: [46867.276259] squid3[22728]: segfault at 0 ip 0055f9b0 sp 7fff7bdecab0 error 4 in squid[40+3be000] Nov 3 20:39:34 nelsonwr kernel: [47192.724355] squid3[24602]: segfault at 0 ip 0055f9b0 sp 7fff275dff30 error 4 in squid[40+3be000] Nov 3 20:45:04 nelsonwr kernel: [47522.764948] squid3[24977]: segfault at 0 ip 0055f9b0 sp 7fff503f74b0 error 4 in squid[40+3be000] I am seeing the following in cache.log at the same time: 2011/11/03 20:39:37 kid1| Starting Squid Cache version 3.2.0.13 for x86_64-pc-linux-gnu... 2011/11/03 20:39:37 kid1| Process ID 24977 2011/11/03 20:39:37 kid1| Process Roles: worker 2011/11/03 20:39:37 kid1| With 65535 file descriptors available 2011/11/03 20:39:37 kid1| Initializing IP Cache... 2011/11/03 20:39:37 kid1| DNS Socket created at 0.0.0.0, FD 7 2011/11/03 20:39:37 kid1| Adding nameserver 89.145.254.78 from /etc/resolv.conf 2011/11/03 20:39:37 kid1| Adding nameserver 94.30.127.100 from /etc/resolv.conf 2011/11/03 20:39:37 kid1| Adding domain wardroom from /etc/resolv.conf 2011/11/03 20:39:37 kid1| helperOpenServers: Starting 5/5 'ext_session_acl' processes 2011/11/03 20:39:37 kid1| Logfile: opening log daemon:/var/log/squid3/access.log 2011/11/03 20:39:37 kid1| Logfile Daemon: opening log /var/log/squid3/access.log 2011/11/03 20:39:37 kid1| Unlinkd pipe opened on FD 23 2011/11/03 20:39:37 kid1| Local cache digest enabled; rebuild/rewrite every 3600/3600 sec 2011/11/03 20:39:37 kid1| Store logging disabled 2011/11/03 20:39:37 kid1| Swap maxSize 0 + 262144 KB, estimated 20164 objects 2011/11/03 20:39:37 kid1| Target number of buckets: 1008 2011/11/03 20:39:37 kid1| Using 8192 Store buckets 2011/11/03 20:39:37 kid1| Max Mem size: 262144 KB 2011/11/03 20:39:37 kid1| Max Swap size: 0 KB 2011/11/03 20:39:37 kid1| Using Least Load store dir selection 2011/11/03 20:39:37 kid1| Set Current Directory to /var/spool/squid3 OS is Debian Squeeze. Thanks for any help, Andy
Re: [PATCH] Add mask option for qos_flows miss parameter
On Tue, 2011-11-01 at 15:22 +1300, Amos Jeffries wrote: > On Mon, 31 Oct 2011 23:33:47 +0000, Andrew Beverley wrote: > > Hi, > > > > The attached patch adds a mask option to the qos_flows miss > > configuration value. The reason for this is to allow the preserved > > mark/TOS value from the server to be altered slightly rather than > > overwritten completely. > > > > Example usage. The following will preserve the netfilter mark, but > > will > > ensure that the (9th) bit specified in the miss value will be set to > > 1 > > in the preserved mark: > > > > qos_flows mark miss=0x100/0xF00 > > > > Andy > > > +1. Looks fine. > > I'm not sure the bad mask needs to be fatal. It could be set to default > mask after the error. If you agree that can be altered on commit and > this go in now. Agreed. Thanks. Andy
Re: Improving qos_flows mark feature - obtaining mark later
On Tue, 2011-11-01 at 12:12 +1300, Amos Jeffries wrote: > On Mon, 31 Oct 2011 22:19:12 +0000, Andrew Beverley wrote: > > On Mon, 2011-10-31 at 15:42 -0600, Alex Rousskov wrote: > >> On 10/31/2011 03:03 PM, Andrew Beverley wrote: > >> > >> > Having thought about this further, I think what I was trying to > >> achieve > >> > was getting the mark every time a packet was received. > >> > >> > >> > I have tried putting the relevant code into > >> > ServerStateData::addVirginReplyBody() which seems to work for > >> different > >> > protocols as well as being called for each packet received. > >> > > >> > Is there any problems with this approach? If not, I will refine > >> and > >> > submit a patch. > >> > >> Yes, if you do not care about HTTP response header or FTP control > >> messages: ServerStateData::addVirginReplyBody() is not called when > >> the > >> HTTP header or FTP control messages are received. If you want to > >> catch > >> those cases (it sounds like you do based on your "every packet" > >> description above), then you need HttpStateData::readReply() and > >> equivalent read handlers in other protocols. > > > > Ah, okay. Thanks for the explanation. > > > >> I do not think there is a single Server method that is always called > >> for > >> every Squid read. You should probably add it and call it from the > >> corresponding HTTP and FTP-specific code. > > > > I assume that I should add it to all the protocols? I may need some > > advice as to the correct functions within each one. Which one do you > > think for FTP? dataRead()? processReplyBody()? > > All the read handlers. With type IOCB or taking CommIoCb* handlers > needs a loot at. Okay. > > If you really want it every packet you can't get any closer than > commHandleRead() in comm.cc which is what schedules those handlers. The > ccb->conn available in there is the connection descriptor being read. It's been established that trying to achieve this is pointless, so I'll either stick with the above, or alternatively reduce it further as below. > > > > >> Please keep in mind that Squid does not work on a TCP packet level. > >> It > >> uses the TCP socket interface and the read(2) may happen when there > >> are > >> multiple packets in the buffer already. > > > > Got you. I was wondering that, as it seemed that the data was being > > buffered somewhere, hence the reason I kept testing my code further > > and > > further back, but to no avail. > > Yes, its buffered in TCP, its buffered in squid, its processed in async > chunk in squid (adaptation means a few more buffers to transit) and its > buffered on write, then its buffered in TCP again. > Each of these steps has a 'random' asynchronous delay as well as > possibly aggregating (parser, chunking) and/or splitting (de-chunking, > adaptation) the data size. > > Overall, I'm a little mystified as to why you need per-packet marks at > all. Some real-time QoS or such? That is all simply outside of Squids > scope of operation. I'm doing both routing and traffic shaping with the mark value. Different bits in the mark value affect each of them. I only want the routing bits present in the connection to the server, otherwise they mess up the routing to the client connection. Routing is done before the packets have hit iptables, so it's too late to change the mark value there. It soon became apparent to me that trying to achieve the above by constantly changing the connection mark value was stupid, and that a better way was to mask the bits instead on the outbound packet, hence the patch I have just submitted. However, having looked into it, I thought that obtaining the mark value could be better anyway, so I thought I would work on that as well. > Squid working with the HTTP per-request metric focus does make a lot > more sense normally to grab the marks at or just after significant MiME > boundaries. Before request-line, before headers, before body, and after > body. Possibly at chunk boundaries. Okay, I'll take your recommendation on that. I don't suppose there is any particular single function that I can target, but instead that it needs to be done within each of those? Thanks, Andy
[PATCH] Add mask option for qos_flows miss parameter
Hi,
The attached patch adds a mask option to the qos_flows miss
configuration value. The reason for this is to allow the preserved
mark/TOS value from the server to be altered slightly rather than
overwritten completely.
Example usage. The following will preserve the netfilter mark, but will
ensure that the (9th) bit specified in the miss value will be set to 1
in the preserved mark:
qos_flows mark miss=0x100/0xF00
Andy
This patch adds a mask option to the qos_flows miss configuration value.
This is to allow the preserved mark/TOS value from the server to be altered
slightly rather than overwritten completely.
Example usage. The following will preserve the netfilter mark, but will
ensure that the (9th) bit specified in the miss value will be set to 1 in
the preserved mark:
qos_flows mark miss=0x100/0xF00
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2011-10-28 19:43:45 +
+++ src/cf.data.pre 2011-10-31 23:00:03 +
@@ -1699,8 +1699,10 @@
parent-hit=0xFF Value to mark hits from parent peers.
- miss=0xFF Value to mark cache misses. Takes precedence
-over the preserve-miss feature (see below).
+ miss=0xFF[/mask] Value to mark cache misses. Takes precedence
+over the preserve-miss feature (see below), unless
+mask is specified, in which case only the bits
+specified in the mask are written.
The TOS variant of the following features are only possible on Linux
and require your kernel to be patched with the TOS preserving ZPH
=== modified file 'src/ip/QosConfig.cc'
--- src/ip/QosConfig.cc 2011-05-13 08:13:01 +
+++ src/ip/QosConfig.cc 2011-10-31 23:30:51 +
@@ -128,12 +128,13 @@
} else if (Ip::Qos::TheConfig.tosParentHit && hierCode==PARENT_HIT) {
tos = Ip::Qos::TheConfig.tosParentHit;
debugs(33, 2, "QOS: Parent Peer hit with hier code=" << hierCode << ", TOS=" << int(tos));
+} else if (Ip::Qos::TheConfig.preserveMissTos) {
+tos = fd_table[conn->fd].tosFromServer & Ip::Qos::TheConfig.preserveMissTosMask;
+tos = (tos & ~Ip::Qos::TheConfig.tosMissMask) | (Ip::Qos::TheConfig.tosMiss & Ip::Qos::TheConfig.tosMissMask);
+debugs(33, 2, "QOS: Preserving TOS on miss, TOS=" << int(tos));
} else if (Ip::Qos::TheConfig.tosMiss) {
-tos = Ip::Qos::TheConfig.tosMiss;
+tos = Ip::Qos::TheConfig.tosMiss & Ip::Qos::TheConfig.tosMissMask;
debugs(33, 2, "QOS: Cache miss, setting TOS=" << int(tos));
-} else if (Ip::Qos::TheConfig.preserveMissTos && Ip::Qos::TheConfig.preserveMissTosMask) {
-tos = fd_table[conn->fd].tosFromServer & Ip::Qos::TheConfig.preserveMissTosMask;
-debugs(33, 2, "QOS: Preserving TOS on miss, TOS=" << int(tos));
}
return setSockTos(conn, tos);
}
@@ -148,12 +149,13 @@
} else if (Ip::Qos::TheConfig.markParentHit && hierCode==PARENT_HIT) {
mark = Ip::Qos::TheConfig.markParentHit;
debugs(33, 2, "QOS: Parent Peer hit with hier code=" << hierCode << ", Mark=" << mark);
-} else if (Ip::Qos::TheConfig.markMiss) {
-mark = Ip::Qos::TheConfig.markMiss;
-debugs(33, 2, "QOS: Cache miss, setting Mark=" << mark);
} else if (Ip::Qos::TheConfig.preserveMissMark) {
mark = fd_table[conn->fd].nfmarkFromServer & Ip::Qos::TheConfig.preserveMissMarkMask;
+mark = (mark & ~Ip::Qos::TheConfig.markMissMask) | (Ip::Qos::TheConfig.markMiss & Ip::Qos::TheConfig.markMissMask);
debugs(33, 2, "QOS: Preserving mark on miss, Mark=" << mark);
+} else if (Ip::Qos::TheConfig.markMiss) {
+mark = Ip::Qos::TheConfig.markMiss & Ip::Qos::TheConfig.markMissMask;
+debugs(33, 2, "QOS: Cache miss, setting Mark=" << mark);
}
return setSockNfmark(conn, mark);
}
@@ -182,12 +184,14 @@
tosSiblingHit = 0;
tosParentHit = 0;
tosMiss = 0;
+tosMissMask = 0;
preserveMissTos = false;
preserveMissTosMask = 0xFF;
markLocalHit = 0;
markSiblingHit = 0;
markParentHit = 0;
markMiss = 0;
+markMissMask = 0;
preserveMissMark = false;
preserveMissMarkMask = 0x;
}
@@ -290,18 +294,37 @@
} else if (strncmp(token, "miss=",5) == 0) {
+char *end;
+
if (mark) {
-if (!xstrtoui(&token[5], NULL, &markMiss, 0, std::numeric_limits::max())) {
+if (!xstrtoui(&token[5], &end, &markMiss, 0, std::numeric_limits::max())) {
debugs(3, DBG_CRITICAL, "ERROR: Bad mark miss value " << &token[5]);
self_destruct();
}
+if (*end == '/') {
+if (!xstrtoui(end + 1, NULL, &markMissMask, 0, std::numeric_limits::max())) {
+debugs(3, DBG_CRITICAL, "ERROR: Bad mark miss mask value " << (end + 1));
+self_destruct();
+}
+} else {
+markMissMask = 0x;
+}
} else {
Re: Improving qos_flows mark feature - obtaining mark later
On Mon, 2011-10-31 at 15:42 -0600, Alex Rousskov wrote: > On 10/31/2011 03:03 PM, Andrew Beverley wrote: > > > Having thought about this further, I think what I was trying to achieve > > was getting the mark every time a packet was received. > > > > I have tried putting the relevant code into > > ServerStateData::addVirginReplyBody() which seems to work for different > > protocols as well as being called for each packet received. > > > > Is there any problems with this approach? If not, I will refine and > > submit a patch. > > Yes, if you do not care about HTTP response header or FTP control > messages: ServerStateData::addVirginReplyBody() is not called when the > HTTP header or FTP control messages are received. If you want to catch > those cases (it sounds like you do based on your "every packet" > description above), then you need HttpStateData::readReply() and > equivalent read handlers in other protocols. Ah, okay. Thanks for the explanation. > I do not think there is a single Server method that is always called for > every Squid read. You should probably add it and call it from the > corresponding HTTP and FTP-specific code. I assume that I should add it to all the protocols? I may need some advice as to the correct functions within each one. Which one do you think for FTP? dataRead()? processReplyBody()? > Please keep in mind that Squid does not work on a TCP packet level. It > uses the TCP socket interface and the read(2) may happen when there are > multiple packets in the buffer already. Got you. I was wondering that, as it seemed that the data was being buffered somewhere, hence the reason I kept testing my code further and further back, but to no avail. Thanks, Andy
Re: Improving qos_flows mark feature - obtaining mark later
On Mon, 2011-10-17 at 10:03 +0200, Kinkie wrote: > On Mon, Oct 17, 2011 at 5:30 AM, Amos Jeffries wrote: > > On 17/10/11 11:23, Andrew Beverley wrote: > >> > >> Hi, > >> > >> I've been using the qos_flows feature for preserving a netfilter mark, > >> but have run into some problems. > >> > >> Currently, the netfilter mark for the connection is obtained in > >> forward.cc, during the stages of opening a connection to the remote > >> server. The problem with this is that the connection mark may change > >> once the reply is received. > >> > >> So, I would like to move Ip::Qos::getNfmarkFromServer() from > >> FwdState::dispatch to a function that is called during the server reply > >> stage. However, I can't work out where it should go. > >> > >> I've looked at moving it to client_side_reply.cc, but I can't find a way > >> to retrieve the remote server connection information. I've also looked > >> at comm_read() in comm.cc, but I can't find the client connection > >> information and it looks like the wrong place anyway. > >> > >> Where is the best place to move it to, where it has access to both the > >> client and server side connection, but where it is late enough to read > >> the mark value if it changes during the server reply? > >> > >> Hope this makes sense! > >> > > > > FwdState::dispatch is called at the start of Server request sending. TO > > start the construction and write of request headers to the server. > > > > For persistent and pinned connections it is essentially in the pause > > position between end of reading one reply and start of sending the next > > request. > > > > I think you mean that it may change on reading the first bytes of reply, > > yes? > > > > In which case the best position is in HttpStateData::processReplyHeader(). > > With matching positions in ftp.cc and tunnel.cc reply starting handlers. > > The server connection is produced by a method dataConnection() of the state > > object. > > The client connection is not exposed. Although it could be. It is in the > > parent ServerStateData::FwdState::clientConn. > > Instead of moving it , why not doing both control points? like > http_access / http_reply_access. > It shouldn't hurt performance, and give greater flexibility.. I don't think that there is much point to having it in both places, as the value is only used when sending data to the client. Something like this will happen: the mark will be retrieved during the request to the server and saved (as now); the mark will then be retrieved again during the server reply; and then the mark will be used to mark packets leaving Squid to the client. I think it makes sense just to retrieve the mark during the server reply stage and use that. Andy
Re: Improving qos_flows mark feature - obtaining mark later
On Mon, 2011-10-17 at 10:34 -0600, Alex Rousskov wrote: > On 10/16/2011 04:23 PM, Andrew Beverley wrote: > > > I've been using the qos_flows feature for preserving a netfilter mark, > > but have run into some problems. > > > > Currently, the netfilter mark for the connection is obtained in > > forward.cc, during the stages of opening a connection to the remote > > server. The problem with this is that the connection mark may change > > once the reply is received. > > > > So, I would like to move Ip::Qos::getNfmarkFromServer() from > > FwdState::dispatch to a function that is called during the server reply > > stage. However, I can't work out where it should go. > > Could you define "server reply stage" more precisely, please? If you > want that function to be called once per [HTTP] response, then the > definition should be that of a single response "state change" and not a > continuous "reply stage". If you want that function to be called once > per connection, then the definition should be based on > connection-specific state changes and not on the response state change. Etc. What I meant was during the receiving of data from the server, as opposed to (currently) during the sending of the request to the server. Having thought about this further, I think what I was trying to achieve was getting the mark every time a packet was received. > > > I've looked at moving it to client_side_reply.cc, but I can't find a way > > to retrieve the remote server connection information. I've also looked > > at comm_read() in comm.cc, but I can't find the client connection > > information and it looks like the wrong place anyway. > > client_side_reply.cc is dealing with receiving responses from Store. > Such responses may have no connection associated with them. If you must > have access to the server connection, then you should look on the server > side (forward.cc, Server.cc, http.cc, ftp.cc, etc. with tunnel.cc being > a special case). See Amos' email for more details. I tried some test code in processReplyHeader() as per Amos' suggestion, but as that function is only called once per reply it doesn't fulfil the requirement of retrieving the mark every time new data arrives. > BTW, code common to HTTP and FTP handling should go into Server.cc to > the extent possible. I have tried putting the relevant code into ServerStateData::addVirginReplyBody() which seems to work for different protocols as well as being called for each packet received. Is there any problems with this approach? If not, I will refine and submit a patch. > > Where is the best place to move it to, where it has access to both the > > client and server side connection, but where it is late enough to read > > the mark value if it changes during the server reply? > > Please note that "during the server reply" may take an hour. Do you want > to do something once per reply, every time there are new bytes read from > the origin server, or every time a new piece of response body is given > to the client side, for example? Ideally it would be for every packet of data received, as each packet of data will have its own mark value. Doing it every time ServerStateData::addVirginReplyBody() is called seems to be pretty close. To give some more detail as to what I was actually trying to achieve: I was trying to mark the *connection* with one value whilst a packet was leaving the internet-facing interface, but a different value as the reply was received. This was because I wanted a different mark value for the packets going to the client from Squid, as to the upstream connection going to the server. However, this just doesn't work very well for reasons that are probably obvious, and I've found with all the code above that the mark value was almost random (presumably because by the time the mark value has been read, the next packet is on its way out). So... I still plan to progress the work above, assuming that you are happy, but my plan for my particular application is to add a mask option for the "qos_flows mark miss" option. I'll send a patch separately once I've finished it. Thanks, Andy
Re: Improving qos_flows mark feature - obtaining mark later
Sorry for the delay, stand by for further emails on this subject as well as the comments below... On Mon, 2011-10-17 at 16:30 +1300, Amos Jeffries wrote: > On 17/10/11 11:23, Andrew Beverley wrote: > > Hi, > > > > I've been using the qos_flows feature for preserving a netfilter mark, > > but have run into some problems. > > > > Currently, the netfilter mark for the connection is obtained in > > forward.cc, during the stages of opening a connection to the remote > > server. The problem with this is that the connection mark may change > > once the reply is received. > > > > So, I would like to move Ip::Qos::getNfmarkFromServer() from > > FwdState::dispatch to a function that is called during the server reply > > stage. However, I can't work out where it should go. > > > > I've looked at moving it to client_side_reply.cc, but I can't find a way > > to retrieve the remote server connection information. I've also looked > > at comm_read() in comm.cc, but I can't find the client connection > > information and it looks like the wrong place anyway. > > > > Where is the best place to move it to, where it has access to both the > > client and server side connection, but where it is late enough to read > > the mark value if it changes during the server reply? > > > > Hope this makes sense! > > > > FwdState::dispatch is called at the start of Server request sending. TO > start the construction and write of request headers to the server. > > For persistent and pinned connections it is essentially in the pause > position between end of reading one reply and start of sending the next > request. > > I think you mean that it may change on reading the first bytes of reply, > yes? Yes. > In which case the best position is in > HttpStateData::processReplyHeader(). What I actually meant was that it could change during the sending of data, as well as the first bytes. I'll explain further in a follow up email. > With matching positions in ftp.cc > and tunnel.cc reply starting handlers. > The server connection is produced by a method dataConnection() of the > state object. > The client connection is not exposed. Although it could be. It is in > the parent ServerStateData::FwdState::clientConn. > > Amos Andy
Re: [PATCH] Session helper: upgrade DB and fix active mode
On Sun, 2011-10-09 at 20:06 +0100, Andrew Beverley wrote: > On Sat, 2011-10-08 at 21:18 +0200, Henrik Nordström wrote: > > fre 2011-10-07 klockan 19:18 +0100 skrev Andrew Beverley: > > > > > I admit that I am rushing to submit this as I go away for the weekend, > > > so please let me know if it's not up to scratch! Works For Me (TM) > > > though. > > > > Why are you allocating the DBT structures? > > With the newer version of Berkeley DB, the structures need to be > initialised to null before use. I thought that in order to do so, I had > to allocate the memory. This was wrong, so I have now just made sure > that they are properly initiated before use in the new patch attached. > > > Also you are not freeing them, causing a memory leak on each lookup. > > > > Good point, although I've remove the malloc now as above. > > I've made a couple of other minor changes - please find updated patch > attached for merge assuming you are happy with it. When using the DB > environment, I have hard-coded the name of the database file within the > directory. I can't see any problems with this, but please let me know if > you disagree (I did originally used program_name, but this doesn't work > when it contains the full path). > > I have also removed the ->sync calls, as these are not required with the > improved synchronisation of the DB environment. Any chance of getting this accepted please? Or is there still some work that I need to do on it? I've been using it for a while now and it seems to work well. Thanks, Andy
Improving qos_flows mark feature - obtaining mark later
Hi, I've been using the qos_flows feature for preserving a netfilter mark, but have run into some problems. Currently, the netfilter mark for the connection is obtained in forward.cc, during the stages of opening a connection to the remote server. The problem with this is that the connection mark may change once the reply is received. So, I would like to move Ip::Qos::getNfmarkFromServer() from FwdState::dispatch to a function that is called during the server reply stage. However, I can't work out where it should go. I've looked at moving it to client_side_reply.cc, but I can't find a way to retrieve the remote server connection information. I've also looked at comm_read() in comm.cc, but I can't find the client connection information and it looks like the wrong place anyway. Where is the best place to move it to, where it has access to both the client and server side connection, but where it is late enough to read the mark value if it changes during the server reply? Hope this makes sense! Thanks, Andy
Re: [PATCH] Session helper: upgrade DB and fix active mode
On Sat, 2011-10-08 at 21:18 +0200, Henrik Nordström wrote:
> fre 2011-10-07 klockan 19:18 +0100 skrev Andrew Beverley:
>
> > I admit that I am rushing to submit this as I go away for the weekend,
> > so please let me know if it's not up to scratch! Works For Me (TM)
> > though.
>
> Why are you allocating the DBT structures?
With the newer version of Berkeley DB, the structures need to be
initialised to null before use. I thought that in order to do so, I had
to allocate the memory. This was wrong, so I have now just made sure
that they are properly initiated before use in the new patch attached.
> Also you are not freeing them, causing a memory leak on each lookup.
>
Good point, although I've remove the malloc now as above.
I've made a couple of other minor changes - please find updated patch
attached for merge assuming you are happy with it. When using the DB
environment, I have hard-coded the name of the database file within the
directory. I can't see any problems with this, but please let me know if
you disagree (I did originally used program_name, but this doesn't work
when it contains the full path).
I have also removed the ->sync calls, as these are not required with the
improved synchronisation of the DB environment.
Finally, I noticed that in doc/manuals/* there is references to the
session helper. Should I be updating these files as well? If so, why is
the information repeated between that location and the session helper
manual in helpers/external_acl/session/?
Thanks,
Andy
This patch makes the following changes to the session helper:
- Removes support for Berkeley DB 1.85
- Adds support for the current Berkeley DB (db.h)
- Adds support for a DB environment (if a directory is specified as
the path then an environment is created). This gives better
synchronisation within multiple processes
- Fixes a bug with active mode where LOGIN/LOGOUT did not write to the DB
=== modified file 'helpers/external_acl/session/ext_session_acl.8'
--- helpers/external_acl/session/ext_session_acl.8 2011-09-21 00:19:19 +
+++ helpers/external_acl/session/ext_session_acl.8 2011-10-09 13:17:47 +
@@ -1,11 +1,11 @@
-.if !'po4a'hide' .TH ext_session_acl 8 "19 September 2011"
+.if !'po4a'hide' .TH ext_session_acl 8 "9 October 2011"
.
.SH NAME
.if !'po4a'hide' .B ext_session_acl
.if !'po4a'hide' \-
Squid session tracking external acl helper.
.PP
-Version 1.1
+Version 1.2
.
.SH SYNOPSIS
.if !'po4a'hide' .B ext_session_acl
@@ -52,9 +52,15 @@
.if !'po4a'hide' .TP
.if !'po4a'hide' .B "\-b path"
.B Path
-to persistent database. If not specified the session details
-will be kept in memory only and all sessions will reset each time
-Squid restarts its helpers (Squid restart or rotation of logs).
+to persistent database. If a file is specified then that single file is
+used as the database. If a path is specified, a Berkeley DB database
+environment is created within the directory. The advantage of the latter
+is better database support between multiple instances of the session
+helper. Using multiple instances of the session helper with a single
+database file will cause synchronisation problems between processes.
+If this option is not specified the session details will be kept in
+memory only and all sessions will reset each time Squid restarts its
+helpers (Squid restart or rotation of logs).
.
.if !'po4a'hide' .TP
.if !'po4a'hide' .B \-a
=== modified file 'helpers/external_acl/session/ext_session_acl.cc'
--- helpers/external_acl/session/ext_session_acl.cc 2011-09-21 00:19:19 +
+++ helpers/external_acl/session/ext_session_acl.cc 2011-10-09 19:02:17 +
@@ -23,19 +23,22 @@
#endif
#include "helpers/defines.h"
-#include
-#include
+#if HAVE_DB_H
+#include
+#endif
#include
+#if HAVE_GETOPT_H
+#include
+#endif
#include
#include
+#include
+#include
+#include
+#include
#if HAVE_UNISTD_H
#include
#endif
-#include
-#include
-#if HAVE_GETOPT_H
-#include
-#endif
/* At this point all Bit Types are already defined, so we must
protect from multiple type definition on platform where
@@ -45,45 +48,71 @@
#define__BIT_TYPES_DEFINED__
#endif
-#if HAVE_DB_185_H
-#include
-#elif HAVE_DB_H
-#include
-#endif
-
static int session_ttl = 3600;
static int fixed_timeout = 0;
char *db_path = NULL;
const char *program_name;
DB *db = NULL;
+DB_ENV *db_env = NULL;
static void init_db(void)
{
-db = dbopen(db_path, O_CREAT | O_RDWR, 0666, DB_BTREE, NULL);
-if (!db) {
-fprintf(stderr, "FATAL: %s: Failed to open session db '%s'\n", program_name, db_path);
-exit(1);
+struct stat st_buf;
+
+if (db_path) {
+if (!stat(db_path, &st_buf)) {
+
[PATCH] Session helper: upgrade DB and fix active mode
Further to discussions, please find attached a patch for the session
helper to:
- Remove support for Berkeley DB 1.85
- Add support for the current Berkeley DB (db.h)
- Add support for a DB environment (fixes synchronisation between
multiple processes)
- Fix the active mode bug previously submitted (but not accepted)
I admit that I am rushing to submit this as I go away for the weekend,
so please let me know if it's not up to scratch! Works For Me (TM)
though.
Thanks,
Andy
This patch makes the following changes to the session helper:
- Removes support for Berkeley DB 1.85
- Adds support for the current Berkeley DB (db.h)
- Adds support for a DB environment (if a directory is specified as
the path then an environment is created). This gives better
synchronisation to multiple processes
- Fixes a bug with active mode where LOGIN/LOGOUT did not write to the DB
=== modified file 'helpers/external_acl/session/ext_session_acl.8'
--- helpers/external_acl/session/ext_session_acl.8 2011-09-21 00:19:19 +
+++ helpers/external_acl/session/ext_session_acl.8 2011-10-07 18:10:31 +
@@ -52,9 +52,15 @@
.if !'po4a'hide' .TP
.if !'po4a'hide' .B "\-b path"
.B Path
-to persistent database. If not specified the session details
-will be kept in memory only and all sessions will reset each time
-Squid restarts its helpers (Squid restart or rotation of logs).
+to persistent database. If a file is specified then that single file is
+used as the database. If a path is specified, a Berkeley DB database
+environment is created within the directory. The advantage of the latter
+is better database support between multiple instances of the session
+helper. Using multiple instances of the session helper with a single
+database file will cause synchronisation problems between processes.
+If this option is not specified the session details will be kept in
+memory only and all sessions will reset each time Squid restarts its
+helpers (Squid restart or rotation of logs).
.
.if !'po4a'hide' .TP
.if !'po4a'hide' .B \-a
=== modified file 'helpers/external_acl/session/ext_session_acl.cc'
--- helpers/external_acl/session/ext_session_acl.cc 2011-09-21 00:19:19 +
+++ helpers/external_acl/session/ext_session_acl.cc 2011-10-07 18:10:27 +
@@ -23,19 +23,22 @@
#endif
#include "helpers/defines.h"
-#include
-#include
+#if HAVE_DB_H
+#include
+#endif
#include
+#if HAVE_GETOPT_H
+#include
+#endif
#include
#include
+#include
+#include
+#include
+#include
#if HAVE_UNISTD_H
#include
#endif
-#include
-#include
-#if HAVE_GETOPT_H
-#include
-#endif
/* At this point all Bit Types are already defined, so we must
protect from multiple type definition on platform where
@@ -45,48 +48,79 @@
#define__BIT_TYPES_DEFINED__
#endif
-#if HAVE_DB_185_H
-#include
-#elif HAVE_DB_H
-#include
-#endif
-
static int session_ttl = 3600;
static int fixed_timeout = 0;
char *db_path = NULL;
const char *program_name;
DB *db = NULL;
+DB_ENV *db_env = NULL;
static void init_db(void)
{
-db = dbopen(db_path, O_CREAT | O_RDWR, 0666, DB_BTREE, NULL);
-if (!db) {
-fprintf(stderr, "FATAL: %s: Failed to open session db '%s'\n", program_name, db_path);
-exit(1);
+struct stat st_buf;
+
+if (db_path) {
+if (!stat(db_path, &st_buf)) {
+if (S_ISDIR (st_buf.st_mode)) {
+/* If directory then open database environment. This prevents sync problems
+between different processes. Otherwise fallback to single file */
+db_env_create(&db_env, 0);
+if (db_env->open(db_env, db_path, DB_CREATE | DB_INIT_MPOOL | DB_INIT_LOCK , 0666)) {
+fprintf(stderr, "FATAL: %s: Failed to open database environment in '%s'\n", program_name, db_path);
+db_env->close(db_env, 0);
+exit(1);
+}
+db_create(&db, db_env, 0);
+}
+}
+}
+
+if (db_env) {
+if (db->open(db, NULL, program_name, NULL, DB_BTREE, DB_CREATE, 0666)) {
+fprintf(stderr, "FATAL: %s: Failed to open db file '%s' in dir '%s'\n",
+program_name, program_name, db_path);
+db_env->close(db_env, 0);
+exit(1);
+}
+} else {
+db_create(&db, NULL, 0);
+if (db->open(db, NULL, db_path, NULL, DB_BTREE, DB_CREATE, 0666)) {
+fprintf(stderr, "FATAL: %s: Failed to open session db '%s'\n", program_name, db_path);
+exit(1);
+}
}
}
static void shutdown_db(void)
{
-db->close(db);
+db->close(db, 0);
+if (db_env) {
+db_env->close(db_env, 0);
+}
}
int session_is_active = 0;
static int session_active(const char *details, size_t len)
{
-DBT key, data;
-key.data = (void *)details;
-key.size = len;
-if (db->get(db, &key, &data, 0) == 0) {
+DBT *key;
+key = (DBT*)malloc(sizeof(DBT));
+DBT
Re: [PATCH] Re: Adding a reset option to the session helper
On Thu, 2011-10-06 at 13:23 +1300, Amos Jeffries wrote: > > Oh, I forgot to say: the DB_ENV functionality requires a *directory* > > rather than a file to be specified for the database (it creates > > several > > files). So, although I don't see a problem with this in principle, it > > does break backwards compatibility. Any thoughts? Just bite the > > bullet? > > > > Andy > > Um. Maybe check for dir/file type on the -b value and use it as base > path? > Or even better check for dir/file type and then either use legacy mode (just a database file, but with the modern DB API) or DB_ENV mode (a directory with several files). Or is that what you meant anyway? Andy
Re: [PATCH] Re: Adding a reset option to the session helper
On Wed, 2011-10-05 at 23:41 +0100, Andrew Beverley wrote: > I have tried using the same functions on the modern API: this didn't fix > the problem (probably because it is just running in compatibility mode). > > I then tried the modern API, but using the added features (DB_ENV). This > *does* fix the problem. > > So, I propose that the session helper is upgraded to both the new API > and to use the DB_ENV functionality. I will write the patch. Any > objections? Oh, I forgot to say: the DB_ENV functionality requires a *directory* rather than a file to be specified for the database (it creates several files). So, although I don't see a problem with this in principle, it does break backwards compatibility. Any thoughts? Just bite the bullet? Andy
Re: [PATCH] Re: Adding a reset option to the session helper
On Thu, 2011-10-06 at 12:07 +1300, Amos Jeffries wrote: > I think 1.85 is so ancient now its not really needed for old-OS > support. The problem is more likely to be adding #if to support db.h and > 4.2 or such for RHEL. Do you mean Berkeley DB 4.2? If so, from the little I have read, I think that the APIs are largely the same between that and later versions. I'll do the patch shortly. > Aiming for the latest API with wrappers for currently aging-out > versions will give the longest forward-compatible period before anyone > needs to touch it again. Agreed. Andy
Re: [PATCH] Re: Adding a reset option to the session helper
> > FWIW, the ssl_crtd daemon which stores generated SSL certificates > > on > > disk does [lock and] reopen the database every time it needs to read > > it. > > This is not efficient, but avoids conflicts and stale info. > > > > If you do not like that simple but inefficient approach, It was more that I thought it wouldn't get past the stringent Squid developers ;-) > you can use > > a > > better database that can handle concurrency (and leave all caching to > > it) OR implement your own custom and efficient database, possibly > > using shared memory. > > > > Disclaimer: I do not remember what database the session helper is > > using > > so I may be overlooking simpler/better options. > > An ancient version of BerkleyDB. Mostly because the *.db API became > much less simple and potentially slower in later versions. I think that in reality, on modern systems, it uses the same library, but in "1.85 compatibility mode". > An upgrade is well overdue, but only really worth doing if this sync > bug is fixed. I have tried using the same functions on the modern API: this didn't fix the problem (probably because it is just running in compatibility mode). I then tried the modern API, but using the added features (DB_ENV). This *does* fix the problem. So, I propose that the session helper is upgraded to both the new API and to use the DB_ENV functionality. I will write the patch. Any objections? The current session helper appears to allow the inclusion of both db_185.h and db.h. However, if the latter is included, it generates lots of compilation errors because the API has changed. The question is: can we just remove the reference to db_185.h and write the code for the modern API (with the sync fixes), or do you want to keep the option to use db_185? The latter will require several #IF statements to select the correct code for the API. I would prefer to just remove it. Andy
[PATCH] Re: Adding a reset option to the session helper
On Tue, 2011-10-04 at 18:59 +0100, Andrew Beverley wrote:
> However, I'm now having problems with multiple instances of the session
> helper writing to the same database. I thought I had fixed this with the
> ->sync option, but it appears not. If I open multiple instances of
> ext_session_acl in terminal windows on the same database, then I do not
> get consistent results between each process. If I do a "LOGIN" on one,
> then I can still get "No session available" on the other. Any ideas why
> this might be? Is it a bug with db-185? Debugging shows that the key
> does not exist in the second process, despite having been written in the
> first.
After further investigation, the problem appears to be that the *read*
database process is being cached (I'm not sure where). So even if the
writing process syncs its changes to the DB file, these aren't reflected
in the process that is reading the DB file.
After a lot of Googling, the only way I can see to fix this is to close
and re-open the database on every read. This is very messy, but does
anyone have any better suggestions?
> I also found another bug with the session helper when used in active
> mode (which might have been created by me). I'll submit a patch once
> I've got a way ahead with the DB.
The problem here was that the "LOGIN" was being written to the database,
as well as the desired key (even though the correct string length was
specified). So it was never actually matching a subsequent query.
Please find attached a patch that fixes both of these. The fix for the
first is messy; the fix for the second is simple, just using strtok
instead of strrchr, so as to force the key to be correctly truncated.
Andy
This patch fixes two problems with the session helper:
1. When using multiple processes with the DB, it did not properly re-read the
database file when the other process makes a change.
2. Active mode was not properly working, due to the wrong key being written to
the database after a login.
=== modified file 'helpers/external_acl/session/ext_session_acl.cc'
--- helpers/external_acl/session/ext_session_acl.cc 2011-09-21 00:19:19 +
+++ helpers/external_acl/session/ext_session_acl.cc 2011-10-04 22:10:31 +
@@ -79,11 +79,16 @@
DBT key, data;
key.data = (void *)details;
key.size = len;
+/* This is very messy, but appears to be the only way to force a reload of the DB.
+ Failure to do this causes sync problems when multiple helper processes are running. */
+shutdown_db();
+init_db();
if (db->get(db, &key, &data, 0) == 0) {
time_t timestamp;
if (data.size != sizeof(timestamp)) {
fprintf(stderr, "ERROR: %s: CORRUPTED DATABASE (%s)\n", program_name, details);
db->del(db, &key, 0);
+db->sync(db, 0);
return 0;
}
memcpy(×tamp, data.data, sizeof(timestamp));
@@ -111,6 +116,7 @@
key.data = (void *)details;
key.size = len;
db->del(db, &key, 0);
+db->sync(db, 0);
}
static void usage(void)
@@ -156,21 +162,19 @@
while (fgets(request, HELPER_INPUT_BUFFER, stdin)) {
int action = 0;
const char *channel_id = strtok(request, " ");
-const char *detail = strtok(NULL, "\n");
+const char *detail = strtok(NULL, " \n");
if (detail == NULL) {
// Only 1 paramater supplied. We are expecting at least 2 (including the channel ID)
fprintf(stderr, "FATAL: %s is concurrent and requires the concurrency option to be specified.\n", program_name);
exit(1);
}
-const char *lastdetail = strrchr(detail, ' ');
+const char *lastdetail = strtok(NULL, " \n");
size_t detail_len = strlen(detail);
if (lastdetail) {
-if (strcmp(lastdetail, " LOGIN") == 0) {
+if (strcmp(lastdetail, "LOGIN") == 0) {
action = 1;
-detail_len = (size_t)(lastdetail-detail);
-} else if (strcmp(lastdetail, " LOGOUT") == 0) {
+} else if (strcmp(lastdetail, "LOGOUT") == 0) {
action = -1;
-detail_len = (size_t)(lastdetail-detail);
}
}
if (action == -1) {
Re: Adding a reset option to the session helper
On Sat, 2011-10-01 at 15:51 +1300, Amos Jeffries wrote: [...] > When the LOGIN/LOGOUT ACLs are tested they perform their action on the > session state. The ACLs leading up to them have to be crafted to avoid > testing them at all unless you want LOGIN/LOGOUT to happen on that > request [...] Thanks, I've kind-of got that working, and will update the wiki in due course. However, I'm now having problems with multiple instances of the session helper writing to the same database. I thought I had fixed this with the ->sync option, but it appears not. If I open multiple instances of ext_session_acl in terminal windows on the same database, then I do not get consistent results between each process. If I do a "LOGIN" on one, then I can still get "No session available" on the other. Any ideas why this might be? Is it a bug with db-185? Debugging shows that the key does not exist in the second process, despite having been written in the first. I also found another bug with the session helper when used in active mode (which might have been created by me). I'll submit a patch once I've got a way ahead with the DB. Andy
Re: Adding a reset option to the session helper
On Thu, 2011-09-29 at 13:49 +0200, Henrik Nordström wrote: > tis 2011-09-27 klockan 07:32 +0100 skrev Andrew Beverley: > > > I'd like to find a way around this. The best way that I can think of is > > to add an option to the session helper, to specify a URL that must be > > present for the timeout to reset. This URL would then be contained in > > the "continue" button on the splash page. > > See the active mode which is intended for this kind of explicit session > start. Thanks Henrik. That looks spot on. > > The URL match is done using Squid acls. > Could you point me in the direction of how to do this please? Looking at the documentation for the external_acl_type it states that "any string specified in the referencing acl will also be included in the helper request line". Is that the way to achieve it? Something like this? external_acl_type session concurrency=100 ttl=3 %SRC /usr/lib/squid3/ext_session_acl -a -T 10800 -b /var/lib/squid/session.db acl existing_users external session acl aclname urlpath_regex -i a_url_that_must_be_clicked LOGIN deny_info http://nelsonwr.wardroom/announce.php?url=%s existing_users http_access deny !existing_users expiry (Sorry, I would just try it out myself, but I don't have a squid instance to hand, and I want to get this up and running as soon as I get back!) Thanks, Andy
Adding a reset option to the session helper
So, I've been using the session helper for a few days now to display a splash page, and on the whole it's working well. However, one of the problems that I have experienced is that there is often something in the background on a user's computer that retrieves something from the web and forces a reset of the timeout (such as automatic security updates, a ticker on a web page and so on). I'd like to find a way around this. The best way that I can think of is to add an option to the session helper, to specify a URL that must be present for the timeout to reset. This URL would then be contained in the "continue" button on the splash page. What do you think? Is there a better way to solve the problem? Would such a patch be accepted? Thanks, Andy
Re: i'm having a problem while compiling squid 3.2.0.12 (since 3.2.0.6) on ubuntu server
On Wed, 2011-09-21 at 07:54 -0600, Alex Rousskov wrote: > On 09/20/2011 11:29 PM, Amos Jeffries wrote: > >>> > >>> In file included from ../src/ssl/support.h:38, > >>> from ssl/ErrorDetailManager.h:4, > >>> from errorpage.cc:42: > >>> ../src/ssl/gadgets.h:39: error: variable or field ×’X509_free_cpp×’ > >>> declared void > >> > >> I believe I have seen this bug as well. Try installing OpenSSL > >> libraries, including development headers (openssl-devel package or > >> similar). > >> > >> Please consider filing this bug using Squid bugzilla. Please mention > >> whether installing OpenSSL helped. > > > > And which version(s), down to the sub-lettering. There seem to be some > > fun changes going on in the headers for the 1.0.0 releases. > > In my case, it was a Squid bug unrelated to OpenSSL versions. As far as > I could tell, we were including some SSL-related files even when SSL was > disabled, but I did not have a chance to fully investigate. > > > > ok so i installed every possilbe openssl lib that i was thinking of using: > > apt-get install libcurl4-openssl-dev libssl-dev libssl0.9.8 libssl0.9.8-dbg > > > > and then i get the following output (some lines where added).: > > > > make[3]: Entering directory `/opt/src/squid-3.2.0.12/src' > > g++ -DHAVE_CONFIG_H > > -DDEFAULT_CONFIG_FILE=\"/opt/squid32012/etc/squid.conf\" > > -DDEFAULT_SQUID_DATA_DIR=\"/opt/squid32012/share\" > > -DDEFAULT_SQUID_CONFIG_DIR=\"/opt/squid32012/etc\" -I.. -I../include > > -I../lib -I../src -I../include -I../libltdl -I../src -I../libltdl -Wall > > -Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -m64 > > -g -O2 -c -o errorpage.o errorpage.cc > > In file included from ../src/ssl/support.h:38, > > from ssl/ErrorDetailManager.h:4, > > from errorpage.cc:42: > > ../src/ssl/gadgets.h:39: error: variable or field ×’X509_free_cpp×’ declared > > void > > Did you reconfigure Squid from scratch after installing OpenSSL? > > > Thank you, > > Alex. I've just run into this bug. It's a problem when compiling Squid *without* any SSL support (and without SSL development files installed), because there is an attempt to include an SSL header file regardless. Please find attached a patch to fix. Andy This patch stops an SSL header file being included when SSL support has not been requested. === modified file 'src/errorpage.cc' --- src/errorpage.cc 2011-09-12 00:31:13 + +++ src/errorpage.cc 2011-09-25 00:28:44 + @@ -39,7 +39,9 @@ #include "auth/UserRequest.h" #endif #include "SquidTime.h" +#if USE_SSL #include "ssl/ErrorDetailManager.h" +#endif #include "Store.h" #include "html_quote.h" #include "HttpReply.h"
Re: [PATCH] Fix session helper "crashing too rapidly"
On Sat, 2011-09-24 at 15:39 +0200, Henrik Nordström wrote: > mån 2011-09-19 klockan 10:49 +1200 skrev Amos Jeffries: > > > The session helper in Squid-3 is concurrent. The user_key is the opaque > > channel-ID. (Probably should be renamed to match the protocol > > documentation). > > http://wiki.squid-cache.org/Features/AddonHelpers#Access_Control_.28ACL.29 > > It's always been using only the concurrent helper protocol version. > Never existed in any other form. Okay, well I've added the concurrent parameter to all the examples in the wiki anyway :) Andy
Re: [PATCH] Fix session helper "crashing too rapidly"
On Wed, 2011-09-21 at 11:01 +1200, Amos Jeffries wrote: > >> > Done. Also, the following page should be updated: > >> > > >> > http://wiki.squid-cache.org/ConfigExamples/Portal/Splash > >> > > >> > I'm happy to do it myself, if you can give me wiki edit rights? > >> > > Enabled. > I've updated the wiki to what I believe is a clearer example, and included the new fixed-timeout feature. Please let me know if you disagree with anything I have written. Thanks, Andy
Re: [PATCH] Fix session helper "crashing too rapidly"
On Thu, 2011-09-22 at 11:10 +1200, Amos Jeffries wrote: > > However, it still leaves the question: what is the best way to log > > errors from the helper. At the moment, even with the patch applied, a > > user will still get "The helpers are crashing too rapidly, need help" if > > they don't specify a concurrency value. I was trying to fix this, as I > > spent hours locating the problem! > > > > The correct way is with ERROR: or in this case "FATAL:" messages sent to > stderr from the helper. The user will get N of that message over a 30 > seconds period, then Squids helpers dying message and abort. > > Why the message is not showing up in your cache.log is a mystery. > Perhapse it was buffered and discarded by the segfault? something like that? I'm still not getting anything, even with the segfault bug fixed. Could you point in the direction of the code where this should happen (sorry, I can't find it), so that I can try and work out why it's not happening for me please? Thanks for your reply to my other post. Andy
Re: [PATCH] Fix session helper "crashing too rapidly"
On Wed, 2011-09-21 at 11:01 +1200, Amos Jeffries wrote: > On Tue, 20 Sep 2011 22:26:20 +0100, Andrew Beverley wrote: > > On Tue, 2011-09-20 at 12:09 +1200, Amos Jeffries wrote: > >> > Do you mean the -d option to the Squid binary? If so, this doesn't > >> > seem > >> > to make any difference; it just prints all the log messages to the > >> > display as well as the log file. > >> > >> -d parameter of the helper binary. stderr is piped to cache.log at > >> level-0. Thus the need for a separate switch to enable lots of > >> debug > >> from the helper. > > > > Sorry if I'm being stupid, but do you mean the actual session helper > > binary or something else? If it's the former, then that obviously > > just > > causes an exit due to an invalid option. > > > > Yes I did. Sorry, being unbelievably stupid this week it seems :(. > Completely overlooked the fact the 'd' parameter and debugs do not > exist there. No problem! However, it still leaves the question: what is the best way to log errors from the helper. At the moment, even with the patch applied, a user will still get "The helpers are crashing too rapidly, need help" if they don't specify a concurrency value. I was trying to fix this, as I spent hours locating the problem! > > >> > Done. Also, the following page should be updated: > >> > > >> > http://wiki.squid-cache.org/ConfigExamples/Portal/Splash > >> > > >> > I'm happy to do it myself, if you can give me wiki edit rights? > >> > > Enabled. Thanks, I'll do that shortly. > Applied the patch with that caveat. > Thanks. And I have another related question, but rather than going high-jacking this thread, I will start a new one in squid-users :) Andy
Re: [PATCH] Fix session helper "crashing too rapidly"
On Tue, 2011-09-20 at 12:09 +1200, Amos Jeffries wrote:
> > Do you mean the -d option to the Squid binary? If so, this doesn't
> > seem
> > to make any difference; it just prints all the log messages to the
> > display as well as the log file.
>
> -d parameter of the helper binary. stderr is piped to cache.log at
> level-0. Thus the need for a separate switch to enable lots of debug
> from the helper.
Sorry if I'm being stupid, but do you mean the actual session helper
binary or something else? If it's the former, then that obviously just
causes an exit due to an invalid option.
> > Done. Also, the following page should be updated:
> >
> > http://wiki.squid-cache.org/ConfigExamples/Portal/Splash
> >
> > I'm happy to do it myself, if you can give me wiki edit rights?
>
> Sure. Just need to know what username you login with.
Great stuff. AndrewBeverley
> -h is reserved for display of "help" / usage() texts.
>
Good point.
> -T would probably be better for an absolute timeout.
>
Changed as suggested.
> ext_session_acl.8:
> * " (unless -h is specified).". Please make a separate sentence.
> Something like "Also supports fixed length timeouts for regular splash
> page displays."
Done.
>(if you can think of any other useful scenario than regular splash
> pages that can be mentioned too.)
Well I've mentioned the possibility of forcing a user to
re-authenticate, but I'm not sure that I'm correct in saying that. Do
you think that is possible?
> * rather than calling the new mode "Hard timeout". It's a "Periodic"
> or "Fixed length" timeout ... something a bit more descriptive like
> that.
Done.
> ** Needs to explain how it interacts with -a mode. ie I would expect
> LOGOUT or timeout to terminate the session. Explicit LOGIN required for
> a new one to start.
Done.
> ext_session_acl.cc: (modulo the -h ==> -T change requested)
> * in getopt() format syntax ':' indicates a value is received for the
> option. What you coded is "t:b:a?h?".
> * I think you should make -t and -T both optional arguments which are
> interchangeable. ("t:T:b:a?")
Done.
Please find updated patch attached.
Andy
This patch adds some additional error messages when concurrency has
not been specified for the session helper.
It also adds a -T timeout option, which gives a fixed-length timeout,
rather than one that times out on user inactivity.
=== modified file 'helpers/external_acl/session/ext_session_acl.8'
--- helpers/external_acl/session/ext_session_acl.8 2010-09-16 13:07:02 +
+++ helpers/external_acl/session/ext_session_acl.8 2011-09-20 21:22:00 +
@@ -1,11 +1,11 @@
-.if !'po4a'hide' .TH ext_session_acl 8 "19 March 2006"
+.if !'po4a'hide' .TH ext_session_acl 8 "19 September 2011"
.
.SH NAME
.if !'po4a'hide' .B ext_session_acl
.if !'po4a'hide' \-
Squid session tracking external acl helper.
.PP
-Version 1.0
+Version 1.1
.
.SH SYNOPSIS
.if !'po4a'hide' .B ext_session_acl
@@ -18,23 +18,39 @@
.SH DESCRIPTION
.B ext_session_acl
maintains a concept of sessions by monitoring requests
-and timing out sessions if no requests have been seen for the idle timeout
-timer.
-.PP
-Intended use is for displaying "terms of use" pages, ad popups etc.
+and timing out sessions. The timeout is based either on idle use (
+.B -t
+) or a fixed period of time (
+.B -T
+). The former is suitable for displaying terms and conditions to a user; the
+latter is suitable for the display of advertisments or other notices (both as a
+splash page - see config examples online). The session helper can also be used
+to force users to re-authenticate.
.
.SH OPTIONS
.if !'po4a'hide' .TP 12
.if !'po4a'hide' .B "\-t timeout"
-.B Timeout
-for any session. If not specified the default is 3600 seconds.
+Idle timeout for any session. The default if not specified (set to 3600 seconds).
+.
+.if !'po4a'hide' .TP
+.if !'po4a'hide' .B "\-T timeout"
+Fixed timeout for any session. This will end the session after the timeout regardless
+of a user's activity. If used with
+.B active
+mode, this will terminate the user's session after
+.B timeout
+, after which another
+.B LOGIN
+will be required.
+.B LOGOUT
+will reset the session and timeout.
.
.if !'po4a'hide' .TP
.if !'po4a'hide' .B "\-b path"
.B Path
to persistent database. If not specified the session details
will be kept in memory only and all sessions will reset each time
-Squid restarts it's helpers (Squid restart or rotation of logs).
+Squid restarts its helpers (Squid restart or rotation of logs).
.
.if !'po4a'hide' .TP
.if !'po4a'hide' .B \-a
@@ -43,12 +59,16 @@
.B LOGIN
, or terminated by the argument
.B LOGOUT
-.PP
Without this flag the helper automatically starts the session after
the first request.
-.
.SH CONFIGURATION
.PP
+The
+.B ext_session_acl
+helper is a concurrent helper; therefore, the concurrency= option
+.B must
+be specified in the configuration.
+.PP
Configuration example using the default automatic mode
.if !'po4
Re: [PATCH] Fix session helper "crashing too rapidly"
On Mon, 2011-09-19 at 10:49 +1200, Amos Jeffries wrote:
> The session helper in Squid-3 is concurrent.
Ah, okay.
> The user_key is the opaque
> channel-ID. (Probably should be renamed to match the protocol
> documentation).
> http://wiki.squid-cache.org/Features/AddonHelpers#Access_Control_.28ACL.29
I've renamed to channel_id
> The correct way to fix this is to detect the segfault case add a stderr
> ERROR: message that the helper is concurrent and requires a config
> update.
I've added a fatal error message to STDERR, but I can't get it to output
anywhere (see below).
> stderr should appear in cache.log whenever sent.
Hmmm, it doesn't seem to be.
> Most of the lines so
> far appear to be debug messages
Does that include the failure to open a database? This is written to
STDERR as per any other message, but it does not make it anywhere.
> , which depend on the -d option to
> display.
Do you mean the -d option to the Squid binary? If so, this doesn't seem
to make any difference; it just prints all the log messages to the
display as well as the log file.
I've tried increasing the log level to 9, but to no avail.
> Alsothe .8 manual needs to mention the concurrency rather than just
> implying it in the example config.
Done. Also, the following page should be updated:
http://wiki.squid-cache.org/ConfigExamples/Portal/Splash
I'm happy to do it myself, if you can give me wiki edit rights?
> The helper version should get a bump
> to 1.1 as well.
Done in the manual. Is it specified anywhere else as well?
I've also added:
- A new option: "-h". This causes a "hard" timeout, regardless of user
activity. This is because, for many uses of a splash page (adverts etc)
one would want the message to displayed every n hours, regardless of
user activity (certainly that is my requirement).
- A db->sync command. I have found that with more than one child
started, that they do not necessarily share data because each child's
data has not been flushed to disk. This fixes that. However, I am not
sure whether it has other implications, such as much greater disk
overhead. Do you think it should be a configurable option?
Is there anything else that needs updating? RELEASENOTES.html?
Please find attached updated patch for comment.
Thanks,
Andy
=== modified file 'helpers/external_acl/session/ext_session_acl.8'
--- helpers/external_acl/session/ext_session_acl.8 2010-09-16 13:07:02 +
+++ helpers/external_acl/session/ext_session_acl.8 2011-09-19 22:42:10 +
@@ -1,11 +1,11 @@
-.if !'po4a'hide' .TH ext_session_acl 8 "19 March 2006"
+.if !'po4a'hide' .TH ext_session_acl 8 "19 September 2011"
.
.SH NAME
.if !'po4a'hide' .B ext_session_acl
.if !'po4a'hide' \-
Squid session tracking external acl helper.
.PP
-Version 1.0
+Version 1.1
.
.SH SYNOPSIS
.if !'po4a'hide' .B ext_session_acl
@@ -19,7 +19,7 @@
.B ext_session_acl
maintains a concept of sessions by monitoring requests
and timing out sessions if no requests have been seen for the idle timeout
-timer.
+timer (unless -h is specified).
.PP
Intended use is for displaying "terms of use" pages, ad popups etc.
.
@@ -34,7 +34,7 @@
.B Path
to persistent database. If not specified the session details
will be kept in memory only and all sessions will reset each time
-Squid restarts it's helpers (Squid restart or rotation of logs).
+Squid restarts its helpers (Squid restart or rotation of logs).
.
.if !'po4a'hide' .TP
.if !'po4a'hide' .B \-a
@@ -43,12 +43,22 @@
.B LOGIN
, or terminated by the argument
.B LOGOUT
-.PP
Without this flag the helper automatically starts the session after
the first request.
.
+.if !'po4a'hide' .TP
+.if !'po4a'hide' .B \-h
+Hard timeout mode. In this mode sessions only last for
+.B timeout
+seconds, regardless of the user's activity.
.SH CONFIGURATION
.PP
+The
+.B ext_session_acl
+helper is a concurrent helper; therefore, the concurrency= option
+.B must
+be specified in the configuration.
+.PP
Configuration example using the default automatic mode
.if !'po4a'hide' .RS
.if !'po4a'hide' .B external_acl_type session ttl=300 negative_ttl=0 children=1 concurrency=200 %LOGIN /usr/local/squid/libexec/ext_session_acl
=== modified file 'helpers/external_acl/session/ext_session_acl.cc'
--- helpers/external_acl/session/ext_session_acl.cc 2010-07-29 14:23:35 +
+++ helpers/external_acl/session/ext_session_acl.cc 2011-09-19 22:42:03 +
@@ -52,6 +52,7 @@
#endif
static int session_ttl = 3600;
+static int hard_timeout = 0;
char *db_path = NULL;
const char *program_name;
@@ -101,6 +102,7 @@
data.data = &now;
data.size = sizeof(now);
db->put(db, &key, &data, 0);
+db->sync(db, 0);
}
static void session_logout(const char *details, size_t len)
@@ -113,10 +115,11 @@
static void usage(void)
{
-fprintf(stderr, "Usage: %s [-t session_timeout] [-b dbpath] [-a]\n", program_name);
+fprintf(stderr, "Usage: %s [-t session_timeout] [-b dbpath] [-a] [-h]\n", progra
[PATCH] Fix session helper "crashing too rapidly"
Hi,
I have run into a problem using the session helper (ext_session_acl). In
its current format, the session helper expects 2 parameters as a
minimum. However, using the example at
http://wiki.squid-cache.org/ConfigExamples/Portal/Splash only one is
passed (the IP address).
The second parameter expected is referred to as the user_key in the
source code, which is then returned as a prefix in the reply. When
user_key is missing, ext_session_acl segfaults. When it *is* there, its
presence in the reply message breaks the protocol (according to
http://www.squid-cache.org/Doc/config/external_acl_type/ the reply
should begin with "OK" or "ERR").
The attached patch completely removes the user_key variable. It Works
For Me (TM), but I do not know the original intention for user_key. Is
it needed?
I would also like to see any STDERR messages from the helper logged to
cache.log (for example, if the database cannot be created). What is the
best way to achieve this? I couldn't work out a way to do it - they
appear to be "lost" at the moment.
Thanks,
Andy
This patch fixes the ability to use the session helper.
Without this patch, the helper expects an additional user_key parameter,
which is then returned in the reply. Both of these appear to break the
current protocol.
=== modified file 'helpers/external_acl/session/ext_session_acl.cc'
--- helpers/external_acl/session/ext_session_acl.cc 2010-07-29 14:23:35 +
+++ helpers/external_acl/session/ext_session_acl.cc 2011-09-18 17:01:29 +
@@ -150,8 +150,7 @@
while (fgets(request, HELPER_INPUT_BUFFER, stdin)) {
int action = 0;
-const char *user_key = strtok(request, " \n");
-const char *detail = strtok(NULL, "\n");
+const char *detail = strtok(request, " \n");
const char *lastdetail = strrchr(detail, ' ');
size_t detail_len = strlen(detail);
if (lastdetail) {
@@ -165,18 +164,18 @@
}
if (action == -1) {
session_logout(detail, detail_len);
-printf("%s OK message=\"Bye\"\n", user_key);
+printf("OK message=\"Bye\"\n");
} else if (action == 1) {
session_login(detail, detail_len);
-printf("%s OK message=\"Welcome\"\n", user_key);
+printf("OK message=\"Welcome\"\n");
} else if (session_active(detail, detail_len)) {
session_login(detail, detail_len);
-printf("%s OK\n", user_key);
+printf("OK\n");
} else if (default_action == 1) {
session_login(detail, detail_len);
-printf("%s ERR message=\"Welcome\"\n", user_key);
+printf("ERR message=\"Welcome\"\n");
} else {
-printf("%s ERR message=\"No session available\"\n", user_key);
+printf("ERR message=\"No session available\"\n");
}
}
shutdown_db();
Re: ext_session_acl helpers crashing
On Sun, 2011-06-05 at 18:12 +0200, Henrik Nordström wrote: > sön 2011-06-05 klockan 17:04 +0100 skrev Andrew Beverley: > > Firstly please tell me if I should be posting this to the users mailing > > list... > > > > I am trying to set up a splash page using the ext_session_acl helper. > > However, I get the error message "FATAL: The ext_session_acl helpers are > > crashing too rapidly, need help!" in the log. > > > > I tried initially using V3.1.2 as provided with Debian, and am now > > trying with V3.2.0.7 > > > > The configuration I am using is as follows: > > > > external_acl_type ext_session_acl ttl=60 %SRC > > /usr/lib/squid3/ext_session_acl -t 7200 -b /etc/squid3/session.db > > /etc/squid3/session.db looks suspicious. The user Squid runs as do not > have write permission there, and it's a temporary file not a > configuration file. > > Should be /var/run/squid/session.db or something like that. And making > sure the user Squid runs as have proper access there. That was pretty stupid of me - blindly copied from the example. However, I still have the same problem. I have changed the db file as suggested (and checked permissions). I have also tried removing the db option altogether: external_acl_type ext_session_acl ttl=60 %SRC /usr/lib/squid3/ext_session_acl -t 7200 But still get the error. Thanks, Andy
ext_session_acl helpers crashing
Firstly please tell me if I should be posting this to the users mailing
list...
I am trying to set up a splash page using the ext_session_acl helper.
However, I get the error message "FATAL: The ext_session_acl helpers are
crashing too rapidly, need help!" in the log.
I tried initially using V3.1.2 as provided with Debian, and am now
trying with V3.2.0.7
The configuration I am using is as follows:
external_acl_type ext_session_acl ttl=60 %SRC /usr/lib/squid3/ext_session_acl
-t 7200 -b /etc/squid3/session.db
acl new_users external ext_session_acl
deny_info http://nelsonwr.wardroom/test.html new_users
http_access deny !new_users
Hopefully the above is correct. I was a little confused with the helper
rename as of V3.2. The man page at
http://www.squid-cache.org/Versions/v3/3.2/manuals/ext_session_acl.html
appears to still use the old configuration example.
I have set the log level to 9, which generates the following entries:
2011/06/05 16:35:39.724 kid1| ACLChecklist::preCheck: 0xa506eb0 checking
'http_access deny !new_users'
2011/06/05 16:35:39.724 kid1| ACLList::matches: checking !new_users
2011/06/05 16:35:39.724 kid1| ACL::checklistMatches: checking 'new_users'
2011/06/05 16:35:39.724 kid1| external_acl.cc(744) aclMatchExternal:
acl="ext_session_acl"
2011/06/05 16:35:39.725 kid1| external_acl.cc(766) aclMatchExternal: No helper
entry available
2011/06/05 16:35:39.725 kid1| aclMatchExternal: ext_session_acl("10.0.10.206")
= lookup needed
2011/06/05 16:35:39.725 kid1| aclMatchExternal: "10.0.10.206": entry=@0, age=0
2011/06/05 16:35:39.725 kid1| aclMatchExternal: "10.0.10.206": queueing a call.
2011/06/05 16:35:39.725 kid1| aclMatchExternal: "10.0.10.206": return -1.
2011/06/05 16:35:39.725 kid1| ACL::ChecklistMatches: result for 'new_users' is
-1
2011/06/05 16:35:39.725 kid1| ACLList::matches: result is false
2011/06/05 16:35:39.725 kid1| aclmatchAclList: 0xa506eb0 returning false (AND
list entry failed to match)
2011/06/05 16:35:39.725 kid1| ACL::FindByName 'new_users'
2011/06/05 16:35:39.725 kid1| ACLChecklist::asyncInProgress: 0xa506eb0 async
set to 1
2011/06/05 16:35:39.725 kid1| externalAclLookup: lookup in 'ext_session_acl'
for '10.0.10.206'
2011/06/05 16:35:39.725 kid1| cbdataLock: 0xa4d6c50=2
2011/06/05 16:35:39.725 kid1| cbdataLock: 0xa506eb0=1
2011/06/05 16:35:39.725 kid1| externalAclLookup: looking up for '10.0.10.206'
in 'ext_session_acl'.
2011/06/05 16:35:39.725 kid1| cbdataLock: 0xa507808=1
2011/06/05 16:35:39.725 kid1| cbdataReferenceValid: 0xa507808
2011/06/05 16:35:39.725 kid1| cbdataLock: 0xa4e8978=3
2011/06/05 16:35:39.725 kid1| cbdataLock: 0xa4e8978=4
2011/06/05 16:35:39.726 kid1| The AsyncCall helperDispatchWriteDone
constructed, this=0xa5088d8 [call55]
2011/06/05 16:35:39.726 kid1| cbdataLock: 0xa4e8978=5
2011/06/05 16:35:39.726 kid1| cbdataUnlock: 0xa4e8978=4
2011/06/05 16:35:39.726 kid1| cbdataUnlock: 0xa4e8978=3
2011/06/05 16:35:39.726 kid1| Write.cc(20) Write: FD 10: sz 12: asynCall
0xa5088d8*1
2011/06/05 16:35:39.726 kid1| ModEpoll.cc(137) SetSelect: FD 10, type=2,
handler=1, client_data=0xb5a7c37c, timeout=0
2011/06/05 16:35:39.726 kid1| helperDispatch: Request sent to ext_session_acl
#1, 12 bytes
2011/06/05 16:35:39.726 kid1| helperSubmit: 10.0.10.206
2011/06/05 16:35:39.726 kid1| externalAclLookup: will wait for the result of
'10.0.10.206' in 'ext_session_acl' (ch=0xa506eb0).
...
2011/06/05 16:35:39.729 kid1| helperHandleRead: 0 bytes from ext_session_acl #1
...
2011/06/05 16:35:39.730 kid1| WARNING: ext_session_acl #1 (FD 10) exited
2011/06/05 16:35:39.730 kid1| Too few ext_session_acl processes are running
(need 1/5)
2011/06/05 16:35:39.730 kid1| leave_suid: PID 18612 called
2011/06/05 16:35:39.730 kid1| storeDirWriteCleanLogs: Starting...
2011/06/05 16:35:39.730 kid1| Finished. Wrote 0 entries.
2011/06/05 16:35:39.730 kid1| Took 0.00 seconds ( 0.00 entries/sec).
FATAL: The ext_session_acl helpers are crashing too rapidly, need help!
Is this a bug or have I done something wrong?
Thanks,
Andy
Re: 3.2 release checkup
On Sat, 2011-05-07 at 17:15 +1200, Amos Jeffries wrote:
> Two months ago we set a goal/checkpoint of early May to have 3.2 on the
> last legs toward release. This is it. Are we on track? No.
>
Would it be possible to get the attached patch applied for the release
please? This makes the compilation options work better for the netfilter
mark patch, which would give a cleaner implementation for the release.
I am still planning to do a patch for pkg-config (for v3.3), but haven't
had a chance yet.
Andy
As it is not possible to get or set a netfilter mark without libcap, this
patch will disable netfilter marking at compilation time if libcap is not
available (in a similar way to Linux transparent proxying).
=== modified file 'configure.ac'
--- configure.ac 2011-05-01 03:03:37 +
+++ configure.ac 2011-05-02 19:02:59 +
@@ -1354,6 +1354,7 @@
dnl Look for libnetfilter_conntrack options (needed for QOS netfilter marking)
+dnl squid_opt_netfilterconntrack is set only when option is explicity specified
AC_ARG_WITH(netfilter-conntrack,
AS_HELP_STRING([--without-netfilter-conntrack],
[Do not use Netfilter conntrack libraries for packet marking.
@@ -1361,7 +1362,7 @@
using --with-netfilter-conntrack=PATH. Default: auto-detect.]), [
case "$with_netfilter_conntrack" in
yes|no)
-: # Nothing special to do here
+squid_opt_netfilterconntrack=$with_netfilter_conntrack
;;
*)
if test ! -d "$withval" ; then
@@ -1371,6 +1372,7 @@
LDFLAGS="-L$squid_opt_netfilterconntrackpath/lib $LDFLAGS"
CPPFLAGS="-I$squid_opt_netfilterconntrackpath/include $CPPFLAGS"
with_netfilter_conntrack=yes
+squid_opt_netfilterconntrack=yes
esac
])
AC_MSG_NOTICE([Linux Netfilter Conntrack support requested: ${with_netfilter_conntrack:=auto}])
@@ -1391,7 +1393,6 @@
with_netfilter_conntrack=yes
fi
fi
-AC_MSG_NOTICE([Linux Netfilter Conntrack support enabled: ${with_netfilter_conntrack} ${squid_opt_netfilterconntrackpath}])
dnl Enable Large file support
@@ -2145,21 +2146,6 @@
AC_MSG_NOTICE([X-Accelerator-Vary support enabled: $enable_x_accelerator_vary])
-AC_ARG_ENABLE(zph-qos,
- AS_HELP_STRING([--enable-zph-qos],[Enable ZPH QOS support]), [
-SQUID_YESNO([$enableval],
-[unrecognized argument to --enable-zph-qos: $enableval])
-])
-SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=yes},
- [Enable Zero Penalty Hit QOS. When set, Squid will alter the
- TOS field of HIT responses to help policing network traffic])
-AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
-if test x"$enable_zph_qos" = "xyes" ; then
-AC_MSG_NOTICE([QOS netfilter mark preservation enabled: $with_netfilter_conntrack])
-SQUID_DEFINE_BOOL(USE_LIBNETFILTERCONNTRACK,${with_netfilter_conntrack:=no},
- [Enable support for QOS netfilter mark preservation])
-fi
-
if $CPPUNITCONFIG --help >/dev/null; then
squid_cv_cppunit_version="`$CPPUNITCONFIG --version`"
AC_MSG_NOTICE([using system installed cppunit version $squid_cv_cppunit_version])
@@ -3233,6 +3219,33 @@
# AC_DEFINEd later
fi
+if test "x$squid_opt_netfilterconntrack" = "xyes" -a "x$with_libcap" != "xyes" ; then
+AC_MSG_ERROR([Linux netfilter conntrack requires libcap support (libcap or libcap2)])
+fi
+if test "x$with_netfilter_conntrack" = "xyes" -a "x$with_libcap" != "xyes" ; then
+AC_MSG_WARN([Missing needed capabilities (libcap or libcap2) for netfilter mark support])
+AC_MSG_WARN([Linux netfilter marking support WILL NOT be enabled])
+with_netfilter_conntrack=no
+fi
+AC_MSG_NOTICE([Linux Netfilter Conntrack support enabled: ${with_netfilter_conntrack} ${squid_opt_netfilterconntrackpath}])
+
+
+AC_ARG_ENABLE(zph-qos,
+ AS_HELP_STRING([--enable-zph-qos],[Enable ZPH QOS support]), [
+SQUID_YESNO([$enableval],
+[unrecognized argument to --enable-zph-qos: $enableval])
+])
+SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=yes},
+ [Enable Zero Penalty Hit QOS. When set, Squid will alter the
+ TOS field of HIT responses to help policing network traffic])
+AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
+if test x"$enable_zph_qos" = "xyes" ; then
+AC_MSG_NOTICE([QOS netfilter mark preservation enabled: $with_netfilter_conntrack])
+SQUID_DEFINE_BOOL(USE_LIBNETFILTERCONNTRACK,${with_netfilter_conntrack:=no},
+ [Enable support for QOS netfilter mark preservation])
+fi
+
+
AC_CHECK_LIB(regex, regexec, [REGEXLIB="-lregex"],[REGEXLIB=''])
AC_ARG_ENABLE(gnuregex,
AS_HELP_STRING([--enable-gnuregex],
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2011-04-11 03:25:32 +
+++ src/cache_cf.cc 2011-05-02 18:54:30 +
@@ -1453,7 +1453,7 @@
}
}
-#if defined(SO_MARK)
+#if SO_MARK && USE_LIBCAP
CBDATA_TYPE(acl_nfmark);
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2011-05-06 14:31:36 +
+++ src/cf.data.pre 2011-05-08 21:13:40 +
Re: Updates to configure.ac for netfilter marking
> > Taking a closer look at the yes/no/auto logics and teh particular reason
> > for changing it I think that is a bug in the SQUID_DEFINE_BOOL. I'm
> > proposing a different simpler change in other discussion thread.
> >
>
> That bit is now has a simpler fix in trunk. You can remove the changes
> to AC_SEARCH_LIBS and AC_CHECK_HEADERS from your patch.
>The rest of it looks okay.
Thanks, please find attached updated patch.
Andy
As it is not possible to get or set a netfilter mark without libcap, this
patch will disable netfilter marking at compilation time if libcap is not
available (in a similar way to Linux transparent proxying).
=== modified file 'configure.ac'
--- configure.ac 2011-01-12 12:54:10 +
+++ configure.ac 2011-01-12 23:09:27 +
@@ -1345,6 +1345,7 @@
dnl Look for libnetfilter_conntrack options (needed for QOS netfilter marking)
+dnl squid_opt_netfilterconntrack is set only when option is explicity specified
AC_ARG_WITH(netfilter-conntrack,
AS_HELP_STRING([--without-netfilter-conntrack],
[Do not use Netfilter conntrack libraries for packet marking.
@@ -1352,7 +1353,7 @@
using --with-netfilter-conntrack=PATH. Default: auto-detect.]), [
case "$with_netfilter_conntrack" in
yes|no)
-: # Nothing special to do here
+squid_opt_netfilterconntrack=$with_netfilter_conntrack
;;
*)
if test ! -d "$withval" ; then
@@ -1362,6 +1363,7 @@
LDFLAGS="-L$squid_opt_netfilterconntrackpath/lib $LDFLAGS"
CPPFLAGS="-I$squid_opt_netfilterconntrackpath/include $CPPFLAGS"
with_netfilter_conntrack=yes
+squid_opt_netfilterconntrack=yes
esac
])
AC_MSG_NOTICE([Linux Netfilter Conntrack support requested: ${with_netfilter_conntrack:=auto}])
@@ -1382,7 +1384,6 @@
with_netfilter_conntrack=yes
fi
fi
-AC_MSG_NOTICE([Linux Netfilter Conntrack support enabled: ${with_netfilter_conntrack} ${squid_opt_netfilterconntrackpath}])
dnl Enable Large file support
@@ -2136,21 +2137,6 @@
AC_MSG_NOTICE([X-Accelerator-Vary support enabled: $enable_x_accelerator_vary])
-AC_ARG_ENABLE(zph-qos,
- AS_HELP_STRING([--enable-zph-qos],[Enable ZPH QOS support]), [
-SQUID_YESNO([$enableval],
-[unrecognized argument to --enable-zph-qos: $enableval])
-])
-SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=yes},
- [Enable Zero Penalty Hit QOS. When set, Squid will alter the
- TOS field of HIT responses to help policing network traffic])
-AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
-if test x"$enable_zph_qos" = "xyes" ; then
-AC_MSG_NOTICE([QOS netfilter mark preservation enabled: $with_netfilter_conntrack])
-SQUID_DEFINE_BOOL(USE_LIBNETFILTERCONNTRACK,${with_netfilter_conntrack:=no},
- [Enable support for QOS netfilter mark preservation])
-fi
-
if $CPPUNITCONFIG --help >/dev/null; then
squid_cv_cppunit_version="`$CPPUNITCONFIG --version`"
AC_MSG_NOTICE([using system installed cppunit version $squid_cv_cppunit_version])
@@ -3224,6 +3210,33 @@
# AC_DEFINEd later
fi
+if test "x$squid_opt_netfilterconntrack" = "xyes" -a "x$with_libcap" != "xyes" ; then
+AC_MSG_ERROR([Linux netfilter conntrack requires libcap support (libcap or libcap2)])
+fi
+if test "x$with_netfilter_conntrack" = "xyes" -a "x$with_libcap" != "xyes" ; then
+AC_MSG_WARN([Missing needed capabilities (libcap or libcap2) for netfilter mark support])
+AC_MSG_WARN([Linux netfilter marking support WILL NOT be enabled])
+with_netfilter_conntrack=no
+fi
+AC_MSG_NOTICE([Linux Netfilter Conntrack support enabled: ${with_netfilter_conntrack} ${squid_opt_netfilterconntrackpath}])
+
+
+AC_ARG_ENABLE(zph-qos,
+ AS_HELP_STRING([--enable-zph-qos],[Enable ZPH QOS support]), [
+SQUID_YESNO([$enableval],
+[unrecognized argument to --enable-zph-qos: $enableval])
+])
+SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=yes},
+ [Enable Zero Penalty Hit QOS. When set, Squid will alter the
+ TOS field of HIT responses to help policing network traffic])
+AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
+if test x"$enable_zph_qos" = "xyes" ; then
+AC_MSG_NOTICE([QOS netfilter mark preservation enabled: $with_netfilter_conntrack])
+SQUID_DEFINE_BOOL(USE_LIBNETFILTERCONNTRACK,${with_netfilter_conntrack:=no},
+ [Enable support for QOS netfilter mark preservation])
+fi
+
+
AC_CHECK_LIB(regex, regexec, [REGEXLIB="-lregex"],[REGEXLIB=''])
AC_ARG_ENABLE(gnuregex,
AS_HELP_STRING([--enable-gnuregex],
=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc 2011-01-08 06:23:27 +
+++ src/cache_cf.cc 2011-01-09 22:12:18 +
@@ -1431,7 +1431,7 @@
}
}
-#if defined(SO_MARK)
+#if SO_MARK && USE_LIBCAP
CBDATA_TYPE(acl_nfmark);
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2011-01-12 05:23:00 +
+++ src/cf.data.pre 2011-01-12 22:28:11 +
@@ -1614,7 +1614,7 @@
NAME: tcp_outgoing_mark
TYPE: acl_nfmark
-
Re: Updates to configure.ac for netfilter marking
> > > > Personally I am quite fine with requiring pkg-config as a build > > requirement for automtic detection of libcap, openssl, openldap and a a > > couple more. My only requirement is that a minimal build should be > > possible even without pkg-config. > > > > pkg-config is often available even on those old OS versions even if not > > normally installed. > > > > Aye, I think similar. > > Andrew, > If you want to make the patch it should be fine for trunk, scheduled > to go out with 3.3. No problem, will do (although I'm away for 3 weeks shortly). Should I keep the above patch separate to the patch previously posted? Or encompass everything together? It would be nice to get the previous patch into 3.2 to prevent anyone having the same problems that I had. Andy
Re: Updates to configure.ac for netfilter marking
On Mon, 2011-01-10 at 22:37 +1300, Amos Jeffries wrote:
> On 10/01/11 19:58, Andrew Beverley wrote:
> > Hi all,
> >
> > I was recently caught out by my own patch when compiling Squid :-)
> > I compiled with netfilter marking enabled, but couldn't work out why
> > packets weren't being marked. It was only after turning on detailed
> > logging that I realised it was because Squid had been compiled without
> > libcap.
> >
> > Therefore, as it is not possible to get or set a netfilter mark without
> > libcap, please find attached a proposed patch which will disable
> > netfilter marking at compilation time if libcap is not available (in a
> > similar way to Linux transparent proxying).
> >
> > I also found a bug in the current configure.ac. You get the message
> > "SQUID_DEFINE_BOOL: unrecognized value for USE_LIBNETFILTERCONNTRACK:
> > 'auto'" if you haven't explicitly set with-netfilter-conntrack. This
> > patch fixes that.
> >
> > Finally, it was recommended by the netfilter guys that as
> > libnetfilter_conntrack offers .pc files, that PKG_CHECK_MODULES should
> > be used to check for its presence. However, having looked at the code
> > for the conntrack program, you'd have to first do a
> > AC_CHECK_PROG(HAVE_PKG_CONFIG). Any thoughts on this please? Should I
> > change the test to PKG_CHECK_MODULES?
> >
> > Thanks,
> >
> > Andy
> >
>
> On the patch:
>
> * "IFDEF: " entries in cf.data.pre needs matching entries/changes in
> cf_gen_defines to produce the documentation "Requires:" details.
Added USE_LIBCAP to SO_MARK.
> * the missing libcap support needs to be a hard MSG_ERROR if
> --with-netfilter-conntrack was specified (xyes) and a MSG_WARN if it was
> not defined (xauto).
>- this patch leaves missing libcap as warn and disable. which is the
> problem you attempt to solve.
Fixed. I've had to add a new variable to the script though
(squid_opt_netfilterconntrack), as the normal variable
(with_netfilter_conntrack) is overwritten if it is auto.
Please find attached updated patch.
Thanks,
Andy
As it is not possible to get or set a netfilter mark without libcap, this
patch will disable netfilter marking at compilation time if libcap is not
available (in a similar way to Linux transparent proxying).
It also fixes a bug in configure.ac that causes "SQUID_DEFINE_BOOL:
unrecognized value for USE_LIBNETFILTERCONNTRACK: 'auto'" if
with-netfilter-conntrack hasn't been explicity state.
=== modified file 'configure.ac'
--- configure.ac 2010-12-26 02:07:17 +
+++ configure.ac 2011-01-10 22:53:14 +
@@ -1345,6 +1345,7 @@
dnl Look for libnetfilter_conntrack options (needed for QOS netfilter marking)
+dnl squid_opt_netfilterconntrack is set only when option is explicity specified
AC_ARG_WITH(netfilter-conntrack,
AS_HELP_STRING([--without-netfilter-conntrack],
[Do not use Netfilter conntrack libraries for packet marking.
@@ -1352,7 +1353,7 @@
using --with-netfilter-conntrack=PATH. Default: auto-detect.]), [
case "$with_netfilter_conntrack" in
yes|no)
-: # Nothing special to do here
+squid_opt_netfilterconntrack=$with_netfilter_conntrack
;;
*)
if test ! -d "$withval" ; then
@@ -1362,23 +1363,23 @@
LDFLAGS="-L$squid_opt_netfilterconntrackpath/lib $LDFLAGS"
CPPFLAGS="-I$squid_opt_netfilterconntrackpath/include $CPPFLAGS"
with_netfilter_conntrack=yes
+squid_opt_netfilterconntrack=yes
esac
])
AC_MSG_NOTICE([Linux Netfilter Conntrack support requested: ${with_netfilter_conntrack:=auto}])
if test "x$with_netfilter_conntrack" != "xno"; then
-AC_SEARCH_LIBS([nfct_query], [netfilter_conntrack],,[
+AC_SEARCH_LIBS([nfct_query], [netfilter_conntrack],with_netfilter_conntrack=yes,[
if test x"$with_netfilter_conntrack" = "xyes"; then
AC_MSG_ERROR([--with-netfilter-conntrack specified but libnetfilter-conntrack library not found])
fi
with_netfilter_conntrack=no])
AC_CHECK_HEADERS([libnetfilter_conntrack/libnetfilter_conntrack.h \
-libnetfilter_conntrack/libnetfilter_conntrack_tcp.h],,[
+libnetfilter_conntrack/libnetfilter_conntrack_tcp.h],with_netfilter_conntrack=yes,[
if test x"$with_netfilter_conntrack" = "xyes"; then
AC_MSG_ERROR([--with-netfilter-conntrack specified but libnetfilter-conntrack headers not found])
fi
with_netfilter_conntrack=no])
fi
-AC_MSG_NOTICE([Linux Netfilter Conntrack support enabled: ${with_netfilter_conntrack} ${squid_opt_netfilterconntrackpath}])
dnl Enable Large file supp
Re: Updates to configure.ac for netfilter marking
On Mon, 2011-01-10 at 22:26 +1300, Amos Jeffries wrote: > On 10/01/11 19:58, Andrew Beverley wrote: > > Hi all, > > > > I was recently caught out by my own patch when compiling Squid :-) > > I compiled with netfilter marking enabled, but couldn't work out why > > packets weren't being marked. It was only after turning on detailed > > logging that I realised it was because Squid had been compiled without > > libcap. > > > > Therefore, as it is not possible to get or set a netfilter mark without > > libcap, please find attached a proposed patch which will disable > > netfilter marking at compilation time if libcap is not available (in a > > similar way to Linux transparent proxying). > > > > I also found a bug in the current configure.ac. You get the message > > "SQUID_DEFINE_BOOL: unrecognized value for USE_LIBNETFILTERCONNTRACK: > > 'auto'" if you haven't explicitly set with-netfilter-conntrack. This > > patch fixes that. > > > > Finally, it was recommended by the netfilter guys that as > > libnetfilter_conntrack offers .pc files, that PKG_CHECK_MODULES should > > be used to check for its presence. However, having looked at the code > > for the conntrack program, you'd have to first do a > > AC_CHECK_PROG(HAVE_PKG_CONFIG). Any thoughts on this please? Should I > > change the test to PKG_CHECK_MODULES? > > I spent a day or so looking into it when Jan suggested it. > > Several of us (myself, Henrik, Robert) have expressed a desire to use > pkg-check in the past. However the state of pkg-check availability is > not clear, so we are not able to convert to it as wanted just yet. > > Squid is expected to build on a number of OS including old Linux > versions which were released before pkg-check became common. I've had no > feedback from anyone using those OS to say it can be ported. So for now > we are being a bit conservative and making do without it. Thanks, I thought that might be the case. Do you mind if I drop your thoughts back to the netfilter-dev mailing list (in answer to Jan's comments), as I feel a bit rude having not replied and not done anything since his email! Thanks, Andy
Updates to configure.ac for netfilter marking
Hi all,
I was recently caught out by my own patch when compiling Squid :-)
I compiled with netfilter marking enabled, but couldn't work out why
packets weren't being marked. It was only after turning on detailed
logging that I realised it was because Squid had been compiled without
libcap.
Therefore, as it is not possible to get or set a netfilter mark without
libcap, please find attached a proposed patch which will disable
netfilter marking at compilation time if libcap is not available (in a
similar way to Linux transparent proxying).
I also found a bug in the current configure.ac. You get the message
"SQUID_DEFINE_BOOL: unrecognized value for USE_LIBNETFILTERCONNTRACK:
'auto'" if you haven't explicitly set with-netfilter-conntrack. This
patch fixes that.
Finally, it was recommended by the netfilter guys that as
libnetfilter_conntrack offers .pc files, that PKG_CHECK_MODULES should
be used to check for its presence. However, having looked at the code
for the conntrack program, you'd have to first do a
AC_CHECK_PROG(HAVE_PKG_CONFIG). Any thoughts on this please? Should I
change the test to PKG_CHECK_MODULES?
Thanks,
Andy
As it is not possible to get or set a netfilter mark without libcap, this
patch will disable netfilter marking at compilation time if libcap is not
available (in a similar way to Linux transparent proxying).
It also fixes a bug in configure.ac that causes "SQUID_DEFINE_BOOL:
unrecognized value for USE_LIBNETFILTERCONNTRACK: 'auto'" if
with-netfilter-conntrack hasn't been explicity state.
=== modified file 'configure.ac'
--- configure.ac 2010-12-26 02:07:17 +
+++ configure.ac 2011-01-09 18:23:12 +
@@ -1366,19 +1366,18 @@
])
AC_MSG_NOTICE([Linux Netfilter Conntrack support requested: ${with_netfilter_conntrack:=auto}])
if test "x$with_netfilter_conntrack" != "xno"; then
-AC_SEARCH_LIBS([nfct_query], [netfilter_conntrack],,[
+AC_SEARCH_LIBS([nfct_query], [netfilter_conntrack],with_netfilter_conntrack=yes,[
if test x"$with_netfilter_conntrack" = "xyes"; then
AC_MSG_ERROR([--with-netfilter-conntrack specified but libnetfilter-conntrack library not found])
fi
with_netfilter_conntrack=no])
AC_CHECK_HEADERS([libnetfilter_conntrack/libnetfilter_conntrack.h \
-libnetfilter_conntrack/libnetfilter_conntrack_tcp.h],,[
+libnetfilter_conntrack/libnetfilter_conntrack_tcp.h],with_netfilter_conntrack=yes,[
if test x"$with_netfilter_conntrack" = "xyes"; then
AC_MSG_ERROR([--with-netfilter-conntrack specified but libnetfilter-conntrack headers not found])
fi
with_netfilter_conntrack=no])
fi
-AC_MSG_NOTICE([Linux Netfilter Conntrack support enabled: ${with_netfilter_conntrack} ${squid_opt_netfilterconntrackpath}])
dnl Enable Large file support
@@ -2132,21 +2131,6 @@
AC_MSG_NOTICE([X-Accelerator-Vary support enabled: $enable_x_accelerator_vary])
-AC_ARG_ENABLE(zph-qos,
- AS_HELP_STRING([--enable-zph-qos],[Enable ZPH QOS support]), [
-SQUID_YESNO([$enableval],
-[unrecognized argument to --enable-zph-qos: $enableval])
-])
-SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=yes},
- [Enable Zero Penalty Hit QOS. When set, Squid will alter the
- TOS field of HIT responses to help policing network traffic])
-AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
-if test x"$enable_zph_qos" = "xyes" ; then
-AC_MSG_NOTICE([QOS netfilter mark preservation enabled: $with_netfilter_conntrack])
-SQUID_DEFINE_BOOL(USE_LIBNETFILTERCONNTRACK,${with_netfilter_conntrack:=no},
- [Enable support for QOS netfilter mark preservation])
-fi
-
if $CPPUNITCONFIG --help >/dev/null; then
squid_cv_cppunit_version="`$CPPUNITCONFIG --version`"
AC_MSG_NOTICE([using system installed cppunit version $squid_cv_cppunit_version])
@@ -3220,6 +3204,30 @@
# AC_DEFINEd later
fi
+if test "x$with_netfilter_conntrack" = "xyes" -a "x$with_libcap" != "xyes" ; then
+AC_MSG_WARN([Missing needed capabilities (libcap or libcap2) for netfilter mark support])
+AC_MSG_WARN([Linux netfilter marking support WILL NOT be enabled])
+with_netfilter_conntrack=no
+fi
+AC_MSG_NOTICE([Linux Netfilter Conntrack support enabled: ${with_netfilter_conntrack} ${squid_opt_netfilterconntrackpath}])
+
+
+AC_ARG_ENABLE(zph-qos,
+ AS_HELP_STRING([--enable-zph-qos],[Enable ZPH QOS support]), [
+SQUID_YESNO([$enableval],
+[unrecognized argument to --enable-zph-qos: $enableval])
+])
+SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=yes},
+ [Enable Zero Penalty Hit QOS. When set, Squid will alter the
+ TOS field of HIT responses to help policing network traffic])
+AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
+if test x"$enable_zph_qos" = "xyes" ; then
+AC_MSG_NOTICE([QOS netfilter mark preservation enabled: $with_netfilter_conntrack])
+SQUID_DEFINE_BOOL(USE_LIBNETFILTERCONNTRACK,${with_netfilter_conntrack:=no
Re: Updated netfilter mark patch
> http://wiki.squid-cache.org/Squid3CodingGuidelines already covers both C++ > and automake policies. I've added a menu to easily navigate the page and > this under autoconf guidelines. > Hmmm, my fault entirely for not looking properly, but I didn't realise that page existed. Can I suggest a link from http://wiki.squid-cache.org/Squid3VCS please? Andy
Re: Updated netfilter mark patch
On Wed, 2010-10-06 at 10:17 +0200, Kinkie wrote: > > Hmm, we at least want to MSG_NOTICE for both cases, with preferrably a hard > > error if its explicitly stated. > > > > This is where the yes/no/auto comes in handy to switch the type of fail > > message. > > Hi guys, > handling of the --with-netfilter-conntrack option seems broken to me. > e.g.: > with_netfilter_conntrack=$withval > Why do that? it's already done by configure automagics.. Yes, good point. > Also, it doesn't properly handle the yes/no/auto tristate, and it > actually defaults to "yes" rather than "auto" which is not in line > with the documentation text. True, but it gets set back to "no" if the libraries are subsequently not found. Not a problem though if Amos has changed it to a better way of doing it. Thanks for the help from everyone implementing the patch. As you probably realised I am something of an amateur coder, but I appreciate all the patience and guidance. The Squid3VCS wiki was very useful; the only minor point is that a note in there to stipulate a commit message before submitting a patch might be useful. Thanks, Andy
Re: Updated netfilter mark patch
On Mon, 2010-10-04 at 18:12 +1300, Amos Jeffries wrote: > On 20/09/10 00:41, Andrew Beverley wrote: > >>>>> I've moved it next to the headers check. I have also removed the error > >>>>> message that was generated if they don't exist. However, this means that > >>>>> if somebody explicitly sets --with-netfilter-conntrack and the libraries > >>>>> don't exist, then it will silently fail. Is this the behaviour we want? > >>>> > >>>> Hmm, we at least want to MSG_NOTICE for both cases, with preferrably a > >>>> hard error if its explicitly stated. > >>> > >>> There'll be the default AC_SEARCH_LIBS notice in any case, and then it > >>> will also be shown later assuming --enable-zph-qos is set. I've just > >>> realised though that by default the QOS functions are disabled. I > >>> thought the new concept was that everything was enabled by default? > >>> Should I change the default to enabled for --enable-zph-qos? > >> > >> Yes please. > > > > Now enabled by default. > > > > I have also decided that (as previously suggested) a miss option would > > be useful in addition to the value preservation for a miss. This allows > > a miss value to be set when Squid has been compiled without > > libnetfilter-conntrack, and also makes it easier to set a miss value if > > you're happy with a consistent value. I have therefore added this into > > the latest patch (attached). The parameter is called 'miss' and it takes > > precedence over the preserve-miss feature. > > > > I've not heard back from my 2 testers yet, and I am yet to use it in > > anger myself, but I will give feedback on all these once I have it. > > > > Andy > > > > Any news? > One of the testers has just now compiled a build with the patch in, so I should hear back from him shortly; the other I have not heard from. I am yet to use it in a production environment myself, but hope to do so soon. > Unless someone has a reason not to I'm going to commit this when the > current trunk build issues are resolved. I certainly wouldn't have any objections :)
Re: Updated netfilter mark patch
On Sun, 2010-09-19 at 04:24 +1200, Amos Jeffries wrote: > On 19/09/10 00:47, Andrew Beverley wrote: > > On Sat, 2010-09-18 at 20:34 +1200, Amos Jeffries wrote: > >> On 18/09/10 09:18, Andrew Beverley wrote: > >>> Hi, > >>> > >>> Please find attached updated netfilter mark (and QOS tidy up) patch. > >>> > >>> It takes into account all the recent feedback, but leaves the > >>> tcp_outgoing_* and clientside_* configuration functions in cache_cf.cc > >>> as discussed on the mailing list. > >>> > >>> It remains not fully tested, but is provided for any further comments. > >>> > >>> Thanks, > >>> > >>> Andy > >>> > >> configure.in: > >> AC_SEARCH_LIBS for the library still needs to be performed. Its > >> happened before that newbies see the missing-header text and copy *only* > >> the header file onto their box. > >>Just dropping it out of the else section next to the headers check > >> should be enough. > > > > I've moved it next to the headers check. I have also removed the error > > message that was generated if they don't exist. However, this means that > > if somebody explicitly sets --with-netfilter-conntrack and the libraries > > don't exist, then it will silently fail. Is this the behaviour we want? > > Hmm, we at least want to MSG_NOTICE for both cases, with preferrably a > hard error if its explicitly stated. There'll be the default AC_SEARCH_LIBS notice in any case, and then it will also be shown later assuming --enable-zph-qos is set. I've just realised though that by default the QOS functions are disabled. I thought the new concept was that everything was enabled by default? Should I change the default to enabled for --enable-zph-qos? I've also now added a $withval check anyway for netfilter-conntrack, which ERRORs if it has been explicitly stated and the library has not been found. > This is where the yes/no/auto comes in handy to switch the type of fail > message. > > >>* They will then need to be shuffled up a bit. The list is meant to be > >> alphabetically on directive name for easier release-notes reading. > > > > Done. (Although the existing windows_ipaddrchangemonitor and > > url_rewrite_children are the wrong way round). > > > > Aye thanks for the reminder. > > > > > >> I see that its because they only depend on SO_MARK. Which implies > >> that qos_flows could be partially detached from libnetfilter-conntrack > >> for the flows which set a fixed mark, only the pass-thru flow actually > >> requires the library yes? > > > > Good point. I have separated the 2 functionalities to SO_MARK and > > USE_LIBNETFILTERCONNTRACK as appropriate and updated the documentation. > > Only mark preservation is now unavailable without > > libnetfilter_conntrack. > > > > One problem though, is that I want to warn people if they are using the > > qos_flows mark functions, if mark preservation is unavailable due to the > > lack of netfilter-conntrack. I have added a warning into the config > > parsing, but debugs() at this stage do not make it into the log files. > > Should I just get rid of this warning or is there a better way of doing > > it? > > It's logged in syslog for people who know to check there on startup. > Displayed to screen for -k check and cache.log on reconfigure. > > A number of other features in this position so don't worry about it. > Fixing that tangle is outside the scope of this update. I couldn't see anything in syslog, unless I'm mistaken. If you're happy as it is though, I'll leave it be. > > >>* The texts about ECN have been found to be incorrect. It's the > >> rightmost bits 6-7 which are masked out for ECN. Not the > >> highest/leftmost 0-1. The trunk code has corrected text for the > >> tcp_outgoing_tos but qos_flows is incorrect still. Please update your > >> changes to their texts to include that fix. > > > > Fixed, although I have actually been unable to set any bits other than > > 3, 4 and 5, so the only values I have been able to use are 0-28 in > > multiples of 4. Does anyone know if this is the case with other > > platforms? If so, that might need mentioning as well. > > Strange that bits 0,1,2 were not available. > > > >> * for the parseConfigLine() *-hit entries you output an ERROR without > >> failing or indicating what recovery action has taken place. > >> ** it _looks_ at face value
Re: Patch to add netfilter mark support
On Thu, 2010-09-16 at 16:23 -0600, Alex Rousskov wrote: > On 09/15/2010 12:12 AM, Andrew Beverley wrote: > > On Wed, 2010-09-15 at 02:06 +, Amos Jeffries wrote: > >> On Tue, 14 Sep 2010 23:55:20 +0100, Andrew Beverley > >> wrote: > >>>> * Config.accessList.outgoingTos, Config.accessList.clientsideTos, > >>>> Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark > >> can > >>>> become members of the Qos scope Config object. All the parsing /free > >>>> stuff > >>>> can be moved there too with some #define parse_...() etc for the legacy > >>>> parser. > >>>> > >>> > >>> I've moved all the configuration variables and functions to the Qos > >>> scope. I have renamed parse_acl_tos(acl_tos ** head) as > >>> Ip::Qos::Config::parseConfigAclTos(acl_tos ** head). > >>> > >>> However, I'm unable to compile because of the following error: > >>> > >>> Qos.cc: In member function ‘void > >>> Ip::Qos::Config::parseConfigAclTos(acl_tos**)’: > >>> Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’ > >> does > >>> not match ‘void (*)(void*)’ > >>> > >>> The code at line 377 is: > >>> > >>> CBDATA_INIT_TYPE_FREECB(acl_tos, freedConfigAclTos); > >>> > >>> I have > >>> > >>> CBDATA_TYPE(acl_tos); > >>> > >>> specified before the parseConfigAclTos function. > >>> > >>> Could you give me any ideas as to what I am doing wrong here? If you > >>> need me to send through any more of the code then please let me know. > >> > >> Do you have this with a cast? > >> #define parse_acl_tos(X) Ip::Qos::Config::parseConfigAclTos((acl_tos > >> **)X) > >> > >> with the cf.data.pre "TYPE: acl_tos" unchanged. > > > > No, I had changed it. However, I have now changed it back to the above, > > but still get the same error. Any other ideas? > > > > Qos.cc: In member function ‘void > > Ip::Qos::Config::parseConfigAclTos(acl_tos**)’: > > Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’ does > > not match ‘void (*)(void*)’ > > > > I have attached my current Qos.cc and Qos.h > > I kind of lost track of the current state. Do you still want help with > making this work? Or did you give up and are now back to the old C-style > craft outside of the proper namespace? I gave up and moved the config functions (for outgoing_tos etc) back to cache_cf.cc. The original QOS config parsing is still in Qos.cc. I thought from what Amos said that it couldn't be made to work with CBDATA, but if you have any suggestions then I'll happily look at them. I'll hopefully send the patch in its current format through later on today anyway, so you can see its current state. Thanks, Andy
Re: Patch to add netfilter mark support
On Wed, 2010-09-15 at 19:44 +1200, Amos Jeffries wrote: > On 15/09/10 18:12, Andrew Beverley wrote: > > On Wed, 2010-09-15 at 02:06 +, Amos Jeffries wrote: > >> On Tue, 14 Sep 2010 23:55:20 +0100, Andrew Beverley > >> wrote: > >>>> * Config.accessList.outgoingTos, Config.accessList.clientsideTos, > >>>> Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark > >> can > >>>> become members of the Qos scope Config object. All the parsing /free > >>>> stuff > >>>> can be moved there too with some #define parse_...() etc for the legacy > >>>> parser. > >>>> > >>> > >>> I've moved all the configuration variables and functions to the Qos > >>> scope. I have renamed parse_acl_tos(acl_tos ** head) as > >>> Ip::Qos::Config::parseConfigAclTos(acl_tos ** head). > >>> > >>> However, I'm unable to compile because of the following error: > >>> > >>> Qos.cc: In member function ‘void > >>> Ip::Qos::Config::parseConfigAclTos(acl_tos**)’: > >>> Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’ > >> does > >>> not match ‘void (*)(void*)’ > >>> > >>> The code at line 377 is: > >>> > >>> CBDATA_INIT_TYPE_FREECB(acl_tos, freedConfigAclTos); > >>> > >>> I have > >>> > >>> CBDATA_TYPE(acl_tos); > >>> > >>> specified before the parseConfigAclTos function. > >>> > >>> Could you give me any ideas as to what I am doing wrong here? If you > >>> need me to send through any more of the code then please let me know. > >> > >> Do you have this with a cast? > >> #define parse_acl_tos(X) Ip::Qos::Config::parseConfigAclTos((acl_tos > >> **)X) > >> > >> with the cf.data.pre "TYPE: acl_tos" unchanged. > > > > No, I had changed it. However, I have now changed it back to the above, > > but still get the same error. Any other ideas? > > > > Qos.cc: In member function ‘void > > Ip::Qos::Config::parseConfigAclTos(acl_tos**)’: > > Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’ does > > not match ‘void (*)(void*)’ > > > > I have attached my current Qos.cc and Qos.h > > > > Thanks, > > > > Andy > > > > Darn CBDATA strikes again. What the hell, lets skip this shuffle change > then and get the rest of the work in separately. So have the configuration variables in Qos class (I already have this working), and keep the config functions in cache_cf.cc? Thanks, Andy
Re: Patch to add netfilter mark support
On Wed, 2010-09-15 at 02:06 +, Amos Jeffries wrote:
> On Tue, 14 Sep 2010 23:55:20 +0100, Andrew Beverley
> wrote:
> >> * Config.accessList.outgoingTos, Config.accessList.clientsideTos,
> >> Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark
> can
> >> become members of the Qos scope Config object. All the parsing /free
> >> stuff
> >> can be moved there too with some #define parse_...() etc for the legacy
> >> parser.
> >>
> >
> > I've moved all the configuration variables and functions to the Qos
> > scope. I have renamed parse_acl_tos(acl_tos ** head) as
> > Ip::Qos::Config::parseConfigAclTos(acl_tos ** head).
> >
> > However, I'm unable to compile because of the following error:
> >
> > Qos.cc: In member function ‘void
> > Ip::Qos::Config::parseConfigAclTos(acl_tos**)’:
> > Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’
> does
> > not match ‘void (*)(void*)’
> >
> > The code at line 377 is:
> >
> > CBDATA_INIT_TYPE_FREECB(acl_tos, freedConfigAclTos);
> >
> > I have
> >
> > CBDATA_TYPE(acl_tos);
> >
> > specified before the parseConfigAclTos function.
> >
> > Could you give me any ideas as to what I am doing wrong here? If you
> > need me to send through any more of the code then please let me know.
>
> Do you have this with a cast?
> #define parse_acl_tos(X) Ip::Qos::Config::parseConfigAclTos((acl_tos
> **)X)
>
> with the cf.data.pre "TYPE: acl_tos" unchanged.
No, I had changed it. However, I have now changed it back to the above,
but still get the same error. Any other ideas?
Qos.cc: In member function ‘void Ip::Qos::Config::parseConfigAclTos(acl_tos**)’:
Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’ does not
match ‘void (*)(void*)’
I have attached my current Qos.cc and Qos.h
Thanks,
Andy
#include "acl/Gadgets.h"
#include "ConfigParser.h"
#include "fde.h"
#include "HierarchyLogEntry.h"
#include "ip/tools.h"
#include "Qos.h"
#include "Parsing.h"
#include "squid.h"
#include "Store.h"
extern void dump_acl_list(StoreEntry * entry, ACLList * head);
/* Qos namespace */
void
Ip::Qos::getTosFromServer(const int server_fd, fde *clientFde)
{
#if USE_QOS_TOS
tos_t tos = 1;
int tos_len = sizeof(tos);
clientFde->tosFromServer = 0;
if (setsockopt(server_fd,SOL_IP,IP_RECVTOS,&tos,tos_len)==0) {
unsigned char buf[512];
int len = 512;
if (getsockopt(server_fd,SOL_IP,IP_PKTOPTIONS,buf,(socklen_t*)&len) == 0) {
/* Parse the PKTOPTIONS structure to locate the TOS data message
* prepared in the kernel by the ZPH incoming TCP TOS preserving
* patch.
*/
unsigned char * pbuf = buf;
while (pbuf-buf < len) {
struct cmsghdr *o = (struct cmsghdr*)pbuf;
if (o->cmsg_len<=0)
break;
if (o->cmsg_level == SOL_IP && o->cmsg_type == IP_TOS) {
int *tmp = (int*)CMSG_DATA(o);
clientFde->tosFromServer = (tos_t)*tmp;
break;
}
pbuf += CMSG_LEN(o->cmsg_len);
}
} else {
debugs(33, 1, "QOS: error in getsockopt(IP_PKTOPTIONS) on FD " << server_fd << " " << xstrerror());
}
} else {
debugs(33, 1, "QOS: error in setsockopt(IP_RECVTOS) on FD " << server_fd << " " << xstrerror());
}
#endif
}
void Ip::Qos::getNfmarkFromServer(const int server_fd, const fde *servFde, const fde *clientFde)
{
#if USE_QOS_NFMARK
/* Allocate a new conntrack */
if (struct nf_conntrack *ct = nfct_new()) {
/* Prepare data needed to find the connection in the conntrack table.
* We need the local and remote IP address, and the local and remote
* port numbers.
*/
Ip::Address serv_fde_local_conn;
struct addrinfo *addr = NULL;
serv_fde_local_conn.InitAddrInfo(addr);
getsockname(server_fd, addr->ai_addr, &(addr->ai_addrlen));
serv_fde_local_conn = *addr;
serv_fde_local_conn.GetAddrInfo(addr);
unsigned short serv_fde_local_port = ((struct sockaddr_in*)addr->ai_addr)->sin_port;
struct in6_addr serv_fde_local_ip6;
struct in_addr serv_fde_local_ip;
if (Ip::EnableIpv6 && serv_fde_local_conn.IsIPv6()) {
serv_fde_local_ip6 = ((struct sockaddr_in6*)addr->ai_addr)->sin6_addr;
nfct_set_attr_u8(ct, ATTR_L3PROTO, AF_INET6);
st
Re: Patch to add netfilter mark support
> * Config.accessList.outgoingTos, Config.accessList.clientsideTos, > Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark can > become members of the Qos scope Config object. All the parsing /free stuff > can be moved there too with some #define parse_...() etc for the legacy > parser. > I've moved all the configuration variables and functions to the Qos scope. I have renamed parse_acl_tos(acl_tos ** head) as Ip::Qos::Config::parseConfigAclTos(acl_tos ** head). However, I'm unable to compile because of the following error: Qos.cc: In member function ‘void Ip::Qos::Config::parseConfigAclTos(acl_tos**)’: Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’ does not match ‘void (*)(void*)’ The code at line 377 is: CBDATA_INIT_TYPE_FREECB(acl_tos, freedConfigAclTos); I have CBDATA_TYPE(acl_tos); specified before the parseConfigAclTos function. Could you give me any ideas as to what I am doing wrong here? If you need me to send through any more of the code then please let me know. Thanks, Andy
Re: Patch to add netfilter mark support
> src/Parsing.*: > * Please palce the strtou*() functions to a file under lib/ with an .h in > include/. At this stage they get linked in globally through lib/libmisc.la. Done, but as they both return bool I've had to add stdbool.h to the headers that the file includes and also add a check for stdbool.h to configure.in. Should I stick with this methodology or is it better that I just change the return type to an int? > * Can you also mention in the function docs the overflow reason for > (re-)defining our own wrappers. Done. > src/cache_cf.cc: > * You include . This needs to be wrapped in #if HAVE_LIMITS and > listed with the other system file includes, not the squid internal > includes. Done. > - If any squid .h requires it to be defined first then it MUST be > included directly by that .h to pass dependency checks. > > * "Changed from sscanf for greater flexibility for inputs and error > checking" comment is not needed. Removed. > * Config.accessList.outgoingTos, Config.accessList.clientsideTos, > Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark can > become members of the Qos scope Config object. All the parsing /free stuff > can be moved there too with some #define parse_...() etc for the legacy > parser. I agree that this would be desirable, but I'm not sure how to do it without things getting messy. Because each of the parameters applies to an access list and not globally, surely they need to stay within the accessList struct, which means I can't move them to the Qos class? I guess I can still move the parsing stuff to Qos though. > Along with Alex comment about protos, I think this unwraps any need of the > QoS stuff to include structs.h,protos.h and typedefs.h > > cf.data.pre: > * the old docs text about the above ACL tests being "based on the > username or source address making the request" is probably quite wrong. > It's based on ANY request/connection criteria is it not? and possibly reply > details as well for the reply packets. Changed to "based on an ACL." > * please also use CIDR masks in the new documentation. class masks should > have been killed long ago. Done. > src/ip/QosConfig.h: > * please wrap the system includes separately, with a check for each in > configure.in. Done. Thanks, Andy
Re: Patch to add netfilter mark support
On Fri, 2010-09-10 at 16:46 +1200, Amos Jeffries wrote: > On 10/09/10 07:08, Andrew Beverley wrote: > > > > >> * ARG_WITH if-yes clause then becomes: > >>case "$withval" in > >> yes|no) with_netfilter_conntrack=$withval ;; > >> *) netfilterconntrackpath=$withval ;; > >>esac > > > > Done. > > > >> * ARG_WITH needs an if-no clause doing with_netfilter_conntrack=no to > >> override the default. > > > > That's in the statement above no? > > no. > > An autoconf if-yes clause handles: >--with-foo >--with-foo=blah > (one of the cases we see sometimes on some weird OS is blah == "no", > that is what gets handled above) > > An autoconf if-no clause handles: >--without-foo >--without-foo=blah > > since we only care that its saying "without", we simply need to > hard-code with_netfilter_conntrack=no when it happens. Hmmm, maybe I'm getting the wrong end of the stick here, but I think specifying --with-foo=no and --without-foo is the same (the macros both set $with_foo=no). From the autoconf manual: "--without-package is equivalent to --with-package=no" I've just tried it now and it seems to work like that. One other thing to note is that setting $with_foo before an AC_ARG_WITH (such as for a default) will force the AC_ARG_WITH to be skipped. Therefore, I have used the action-if-not-given functionality of AC_ARG_WITH to set a default value. Regards, Andy
Re: Patch to add netfilter mark support
On Mon, 2010-09-06 at 02:53 +, Amos Jeffries wrote:
> On Sun, 05 Sep 2010 21:59:34 +0100, Andrew Beverley
> wrote:
> > Please find attached the latest version of the patch to add Netfilter
> > marking support to Squid.
> >
> > All the previous comments have now been actioned.
> >
> > One thing that I haven't dealt with yet is the dependency on the
> > ip_conntrack kernel module. This seems to get loaded automatically after
> > some use of Squid, but not straight away, which means that the mark
> > retention does not initially work. I've done some googling but have not
> > found how to force a kernel module load in a program. Is someone able to
> > advise please?
> >
> > Since my last submission (prompted by a request on squid-users), I have
> > also added tcp_outgoing_mark and clientside_mark to complement
> > tcp_outgoing_tos and clientside_tos. I am conscious that I have been
> > copying old code to implement these, some of which does not seem
> > particularly elegant. However, rather than changing things from my
> > inexperienced perspective, I thought it best if I post the changes as-is
> > and action any feedback as appropriate.
> >
> > As part of this I have added isAclNfmarkActive() and isAclTosActive() to
> > return whether there should be any active TOS or MARK packet marking. I
> > added these to fde as that seemed the most appropriate place, but again
> > please tell me if I should move them elsewhere.
> >
> > Thanks,
> >
> > Andy
>
> I'm unable to check the configure.in logics at present; however they
> should be operating to auto-detect libnetfilter_contrack unless --without
> is specified. correct?
Yes.
> configure.in:
> The feature should be defaulting to auto-on. This means...
>
> * A default value for with_netfilter_conntrack=yes and maybe unset
> netfilterconntrackpath needs to be set before the ARG_WITH check.
with_netfilter_conntrack=yes now set first
netfilterconntrackpath is only set if a path has been specified
> Although
> I believe under the new design that latter should be
> squid_opt_netfilterconntrackpath
Done.
> * Help text needs to be talking about using --without-* to remove/disable
> the library use (and via that the feature). Also I think the text
> description you have there is more suited to the release notes detailed
> explanation of the new option. The ./configure can state just this:
> [--without-netfilter-conntrack],[Do not use Netfilter conntrack libraries
> for packet marking.
> A path to alternative library location may be specified.
> Default: auto-detect.]
Done, although with slightly more detail as to setting the path.
> * ARG_WITH if-yes clause then becomes:
> case "$withval" in
> yes|no) with_netfilter_conntrack=$withval ;;
> *) netfilterconntrackpath=$withval ;;
> esac
Done.
> * ARG_WITH needs an if-no clause doing with_netfilter_conntrack=no to
> override the default.
That's in the statement above no?
> * Then the enable/disable test becomes: 'if test
> "x$with_netfilter_conntrack" != "xno"; '
Done with xyes to make more readable
> * The alternate path is only an alternative source of the
> libnetfilter-conntrack correct? that means the "else" part of the if
> statement needs to be run regardless of what the flags contain. The flags
> will merely cause that source to be used if needed.
Done. So if people want to specify an alternative location for the
header files, do they just do that with the appropriate CPP flags?
I was working on the basis that the path specified using
--with-netfilter=PATH would be the path to the library *and* the
headers. I've now changed it to just the library.
> * The error about a directory being "not executable" is incorrect. using
> -d and "does not exist".
Fixed.
> * please replace AC_CHECK_LIB with AC_SEARCH_LIBS and set
> with_netfilter_conntrack=no when it emits that error.
Done.
> * AC_CHECK_HEADERS is needed regardless of whether the system or a custom
> library is used.
Done.
> * With default-on the AC_CHECK_HEADERS gets two extra parameters:
> AC_CHECK_HEADERS[ ... ],[:],[with_netfilter_conntrack=no])
> which cleanly disables the feature if any header is missing.
Done.
> @line 2046:
> * please us the X prefix on string tests for 'if test "$enable_zph_qos" =
> "yes"'
Done.
> * you don't need the second if statement. 'if test
> "$with_netfilter_conntrack" = "yes"'
Done.
> * SQUID_DEFINE_BOOL instead with ${with_netfilter_conntrack:=no} as the
> value to set.
Done.
> * Just the notice about the marking yes/no state is sufficient,
> interested people can see that QoS was enabled but marking part of it
> disabled the very next line.
Understood.
Comments on remainder of source code will follow later...
Andy
Re: Patch to add netfilter mark support
> * I find the terminology inconsistent and confusing: outgoing, > clientside, upstream. No wonder you have to explain the difference > twice. Unless these are all standard RFC-like terms, please use > something consistent like fromClient, toClient, fromServer, toServer. > Others may suggest a better scheme, but this one at least does not > require constant doc lookups to understand where "out" and "up" is. All variables changed as suggested; squid.conf configuration parameters unchanged. > * TOS (unsigned char) and mark (uint32_t) types are repeated numerous > times throughout the patch. Do you thin it would be a good idea to > typedef them? It seems fairly easy for somebody to type them wrong or > mix them up. I cannot insist on this change, but it would be the right > thing to do, IMO. Changed to tos_t and nfmark_t > * If TOS and mark types can be the same uint32_t, we should probably use > the same type and avoid at least some of the code duplication due to the > difference in types. Kept different as per Amos' email. > * The isAclNfmarkActive() and isAclTosActive() functions appear to be > unrelated to src/fde.cc. They are about configuration, right? Why not > make them Qos globals or, of the lists are moved, Qos::Config methods? Moved to Qos::Config, although renamed the existing isMarkActive to differentiate. > * I believe Amos does not want new functions in protos.h unless > necessary. Can you declare getOutgoingTOS() and getOutgoingNfmark() in > src/forward.h? Moved to forward.h > And capitalize the first letter since they are global? No longer global; moved within FwdState. > * Did you verify that "make -k check" and "./test-builds.sh" did not > break because of your changes? No... but in progress as I write. > > * Please remove the following redundant lines (no need to document what > standard methods are): > > > +/** > > + * Constructor > > + */ > > > +/** > > + * Destructor > > + */ Done. > * s/Returns true or false depending on whether/whether/g Done. > * Not sure, but perhaps s/whether we should carry out the QOS functions > for/whether to apply QOS functions to/. This would allow you to collapse > the comment to just one line. Changed to something even better :) > * Closing paren missing in isAclNfmarkActive() description. Done. > * You can use /// comments for /** one line comments */ to make them a > little shorter and avoid line wrapping. Done. Patch to follow shortly. Thanks, Andy
Re: Long time user, first time developer!
Hi Scott, > Anyway, joining this list because I am scoping a project which involves > implementing a captive portal to manage Internet access for unknown > users (the general public) for a "free wifi" hotspot. > > I realise I'm not the first person to set up a captive portal for a free > wifi hotspot, and it's easily achievable using other products that > already exist (they're probably using Squid themselves) but the > requirements we have stretch the boundaries of the other products, hence > the need to develop with Squid first hand. As you're probably already aware, Squid has a built-in landing page functionality: http://wiki.squid-cache.org/ConfigExamples/Portal/Splash However, you might also like to know that I was in a similar situation to you a while ago, where I couldn't find a captive portal system to do exactly what I wanted. I therefore just implemented my own using iptables rules and PHP scripts, all detailed at: http://www.andybev.com/index.php/Using_iptables_and_PHP_to_create_a_captive_portal I look forward to hear more about your actual requirements. Regards, Andy
Re: Patch to add netfilter mark support
On Mon, 2010-09-06 at 23:06 +0100, Andrew Beverley wrote: > > > > > >> * I find the terminology inconsistent and confusing: outgoing, > > >> clientside, upstream. No wonder you have to explain the difference > > >> twice. Unless these are all standard RFC-like terms, please use > > >> something consistent like fromClient, toClient, fromServer, toServer. > > >> Others may suggest a better scheme, but this one at least does not > > >> require constant doc lookups to understand where "out" and "up" is. > > > > > > Agreed. This confusion is also present in the names of the configuration > > > parameters: initially I found the current ones confusing (it took me a > > > while to realise that one was server side and one client side). > > > > > > At the minute they are tcp_outgoing_tos and clientside_tos. Would there > > > be any objection to changing the tcp_outgoing_tos to serverside_tos? Or > > > would you prefer not to break existing squid.conf configurations? > > > > IMHO, both: Change the documented/primary option names but accept the > > old ones with a "deprecated" warning. There may even be a built-in > > mechanism for that (multiple NAME values?), but I am not sure. > > Ah yes, you can specify multiple NAME values. Funnily enough, this is > already the case for tcp_outgoing_tos, which is also known as > tcp_outgoing_ds and tcp_outgoing_dscp. The disadvantage of this is that > it doesn't display a deprecated warning. > > > You probably want to wait for others to comment before changing > > squid.conf option names though. > > How about I change the "default" name to serverside_tos, and leave > tcp_outgoing_tos with tcp_outgoing_ds and tcp_outgoing_dscp as an > accepted name? Or maybe they should be serverside_outgoing_tos and clientside_outgoing_tos to make it even clearer? Andy
Re: Patch to add netfilter mark support
> > > >> * I find the terminology inconsistent and confusing: outgoing, > >> clientside, upstream. No wonder you have to explain the difference > >> twice. Unless these are all standard RFC-like terms, please use > >> something consistent like fromClient, toClient, fromServer, toServer. > >> Others may suggest a better scheme, but this one at least does not > >> require constant doc lookups to understand where "out" and "up" is. > > > > Agreed. This confusion is also present in the names of the configuration > > parameters: initially I found the current ones confusing (it took me a > > while to realise that one was server side and one client side). > > > > At the minute they are tcp_outgoing_tos and clientside_tos. Would there > > be any objection to changing the tcp_outgoing_tos to serverside_tos? Or > > would you prefer not to break existing squid.conf configurations? > > IMHO, both: Change the documented/primary option names but accept the > old ones with a "deprecated" warning. There may even be a built-in > mechanism for that (multiple NAME values?), but I am not sure. Ah yes, you can specify multiple NAME values. Funnily enough, this is already the case for tcp_outgoing_tos, which is also known as tcp_outgoing_ds and tcp_outgoing_dscp. The disadvantage of this is that it doesn't display a deprecated warning. > You probably want to wait for others to comment before changing > squid.conf option names though. How about I change the "default" name to serverside_tos, and leave tcp_outgoing_tos with tcp_outgoing_ds and tcp_outgoing_dscp as an accepted name? Andy
Re: Patch to add netfilter mark support
Thanks for the (very) prompt response (I'm impressed). Replies to other comments to follow; in the meantime... > * I find the terminology inconsistent and confusing: outgoing, > clientside, upstream. No wonder you have to explain the difference > twice. Unless these are all standard RFC-like terms, please use > something consistent like fromClient, toClient, fromServer, toServer. > Others may suggest a better scheme, but this one at least does not > require constant doc lookups to understand where "out" and "up" is. Agreed. This confusion is also present in the names of the configuration parameters: initially I found the current ones confusing (it took me a while to realise that one was server side and one client side). At the minute they are tcp_outgoing_tos and clientside_tos. Would there be any objection to changing the tcp_outgoing_tos to serverside_tos? Or would you prefer not to break existing squid.conf configurations? > [Hint: In most cases, you can quickly rename things if you undo a patch, > change the names in the patch file, and apply the changed patch.] Thanks for the top tip :) Andy
Re: [MERGE] Initial netfilter mark patch for comment
> > The above configure concept would tie in with removing the --enable-qos > > option altogether. There's no reason for the QOS code not to be included > > that I can see (it has no dependencies, apart from the optional upstream > > kernel patch), and with this patch and the isTosActive(), it's enabled > > at runtime only when needed anyway. Is there any reason to keep the > > option? > > This argument has bee put forward for other small features. Sometimes it > succeeds, sometimes not. I know that a mere 1KB saving on binary size > can be extremely useful for embeded devices so the option is likely to > be wanted by somebody. Understood. With the patch in its current format though, I have removed a lot of the #if statements (based on previous feedback) and replaced with is*Active() statements. I'll send the patch through shortly and you can see what you think in its current state. Andy
Re: [MERGE] Initial netfilter mark patch for comment
On Fri, 2010-08-13 at 18:19 -0600, Alex Rousskov wrote:
> On 08/11/2010 03:25 PM, Andrew Beverley wrote:
>
> > I've moved these, as well as most of the other QOS functions, into
> > Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem
> > to fit with all these additional functions.
>
> * A patch preamble with the proposed commit message would be nice.
>
> * I am not sure what Qos class is. It is not documented. If it is a "QOS
> configuration" class, I understand why we have a global instance of it,
> but the original QosConfig name seems better in that case. If it is not
> a configuration class, then I am not sure why we have a global instance
> of it. And the QosConfig file name does not seem to match the class name
> any more.
>
> Perhaps we need two classes, one for configuration and one for
> manipulation? Or a Qos namespace with a configuration class and global
> manipulation functions? The latter seems more likely.
The latter has now been implemented.
> * My understanding is that class data members and public class methods
> should be documented in the header. Others should be documented in the
> .cc files. You may want to double check this rule with Amos before
> moving comments though.
Comments moved to the header in Doxygen format.
> * Many Qos data members are not documented, including new ones.
Now documented, and underscore removed from names.
> * Pass HierarchyLogEntry by const reference, avoid copying. Once that is
> done, move #include "HierarchyLogEntry.h" to the .cc file.
Done.
> * Do you need #include "fde.h" in src/ip/QosConfig.h or can you
> pre-declare fde and include fde.h in the .cc file?
Now pre-declared.
> * s/ >0/ > 0/
Done.
> * This code:
>
> > +if (tos_local_hit || tos_sibling_hit || tos_parent_hit ||
> > preserve_miss_tos) {
> > +return true;
> > +} else {
> > +return false;
> > +}
>
> can be simply written as
>
> return tos_local_hit || tos_sibling_hit || tos_parent_hit ||
> preserve_miss_tos;
>
> Same for Ip::Qos::isMarkActive code.
Done.
>
> * Ip::Qos::isTosActive and Ip::Qos::isMarkActive should be const. When
> that is fixed, you would be able to return const to
> Ip::Qos::dumpConfigLine, I guess.
Done.
> * Ip::Qos::getNfmarkLocalMiss and many other get*() methods should be const.
>
Most of these have now been moved out of classes.
> * What is the purpose of memsetting Qos members to zero in the
> destructor? Please remove the destructor itself if there is no reason to
> reset the memory before freeing it.
Removed.
> * Use Doxygen ///< comments when documenting members, such as upstreamTOS.
Done.
> * Do you need an L suffix for large unsigned constants like 0x?
> Please investigate. I do not know the answer, but I recall seeing such
> suffixes elsewhere:
> http://www.google.com/search?q=0x+vs+0xL
>From what I can gather, the suffix is important in places like 'if'
statements when there is no assignment to a variable with a type (so
that you are comparing like with like). Therefore, I have added the U
suffix to 'if' statements, but I see no need when assigning values to
defined variables (although there would of course be no harm in doing
so).
Regards,
Andy
Re: [MERGE] Initial netfilter mark patch for comment
> > > > * Do you need an L suffix for large unsigned constants like 0x? > > Please investigate. I do not know the answer, but I recall seeing such > > suffixes elsewhere: > > http://www.google.com/search?q=0x+vs+0xL > > I thought that indicated "long" type to be used. When stored as 64-bit > values. > > Which brings up a question of whether it really is an architecture > dependent int field or uint32_t for mark. You're right, it's a fixed size. I've checked the kernel code and marks are defined as u_int32_t/uint32_t. I'll change to uint32_t throughout. Andy
Re: [MERGE] Initial netfilter mark patch for comment
My latest revision of the patch for netfilter marking will follow soon. Before I post it, I will reply to comments on the previous version. > >> > > >> > Question number 2: what is stubQosConfig.cc? Does that also need > >> > updating for this patch? > >> > > >> > >> stub* are cut down set of all non-inline Ip::QosConfig methods and any > >> globals defined in QosConfig.h. Changes to the API need to be mirrored > >> there. The functions inside usually call fatal() to alert a wrong > >> linkage clearly during testing. In this particular case the parse > >> function will need to be duplicated there. > > > > Sorry, I need some clarification on this. I've looked at most of the > > other stub files, and most of the functions do indeed just return > > fatal(). However, the existing stubQosConfig.cc is almost identical to > > QosConfig.cc, with almost all of its code. Shall I change the functions > > to fatal()? > > > > Presumably I should add all the new functions into it (getTosLocalMiss, > > getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS, > > getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)? > > It does need another looking at and possibly stripping down. IIRC it was > there from when QosConfig was a member of SquidConfig. I'll run a quick > check myself now to see what breaks in the current usage if the currently > defined stubs are stripped down. > > You will need to add the appropriate dead-end stubs for the new functions. I've added stubs for all the non-inline functions. They just all call fatal(). If I've not got this quite right then please let me know. > > >> > >> In this version the new methods look they should be in the Ip::Qos > >> namespace. > >> > >> * the new clientReplyContext::tosLocalMissValue() and > >> clientReplyContext::nfmarkLocalMissValue() methods particularly look > >> like they really should take the hier code as a second parameter. Both > >> fd and the hier if passed in should be const. > > > > I've moved them to Ip::Qos and added the parameters as const. > > > >> * the new FwdState getUpstream* methods are in a similar position but > >> not quite so clear cut. If you can find way to cleanly move them there > >> great, otherwise it's not so much a bit deal yet. > > > > I've moved these, as well as most of the other QOS functions, into > > Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem > > to fit with all these additional functions. > > We are moving the rest of Squid in general towards a model of > Namespace::TheConfig containing the config data values pulled from > squid.conf separate from the code which utilizes them. > > Please wait for Alex as well on this particular change. > > My thoughts on it are these; > > I like the moving of functions to IP::Qos including the existing ones. > Under the coding guidelines the existence of class Qos calls for it to be > in a file src/ip/Qos.h and .cc. I have renamed the files from QosConfig to Qos. As per previous discussions, the functions are now contained in a Qos namespace, and the configuration (and functions) are kept in a Config class. > Several of the functions particularly the is*Active() can be inlined for > better performance with _SQUID_INLINE_, which calls for a ip/Qos.cci file > to define them and be conditionally included in the .h or .cc based on #if > _USE_INLINE_. I've moved what I consider to be the most appropriate functions inline. If you disagree with my choices then please tell me. > Some set*Mark and set*Tos functions woudl round out the API. To take the > same parameters as the get*() ones, but which do the perform the > setsockopt. So that paired lines of: > unsigned int mark = Ip::Qos.getNfmarkLocalMiss(fd, > http->request->hier); > comm_set_nfmark(fd,mark); > become > Ip::Qos.setNfmarkLocalMiss(fd, http->request->hier); > > ... and thus comm.cc does not need anything about the MARK setsockopt. > Mirroring the way it has nothing to do with the MARK/TOS getsockopt etc. As per other emails, I have removed get*s and set*s and replaced with do*s. I did experiment with keeping get*s and set*s and moving the get*s to private space, but in all honesty, it just complicated the code without achieving much. I think it's quite tidy now. > > > > I've carried out a fair bit of testing on the mark functionality today, > > and It Works For Me, but I'll be able to try it better in a production > > server in the coming weeks. > > > > Could you let me know if/what else I need to do before acceptance. I > > believe there are the following: > > > > - Confirm and implement stub function requirements > > - Factor the configure.in changes into Kinkie's autoconf-refactor? > > Those are the big ones. Along with getting the namespace change approved. > > The reconfigure straightening will change your dependency logic model a > bit. Specifically such that the absence of --with-netfilter-conntrack > (implicit / auto-detec
Format of variable names
Quick question please: Should variables be named in the form outgoingNetfilterMark or outgoing_netfilter_mark, or does it not matter? There appears to be a variety of formats in use in the existing code. Thanks, Andy
Re: [MERGE] Initial netfilter mark patch for comment
> * My understanding is that class data members and public class methods
> should be documented in the header. Others should be documented in the
> .cc files. You may want to double check this rule with Amos before
> moving comments though.
>
> * Many Qos data members are not documented, including new ones.
>
I have documented all the functions and class data members. Could you
clarify whether *every* variable should be documented with doxygen
comments (including short-lived temporary ones within functions), or
just those that are part of classes/structs?
For example, should 'tos' in the function below have doxygen comments?
int
Ip::Qos::doTosLocalMiss(const int fd, const HierarchyLogEntry *hier)
{
unsigned char tos = 0;
if (Ip::Qos::TheConfig.tos_sibling_hit && hier->code==SIBLING_HIT ) {
tos = Ip::Qos::TheConfig.tos_sibling_hit;
debugs(33, 2, "QOS: Sibling Peer hit with hier code=" << hier->code <<
", TOS=" << (int)tos);
} else if (Ip::Qos::TheConfig.tos_parent_hit && hier->code==PARENT_HIT) {
tos = Ip::Qos::TheConfig.tos_parent_hit;
debugs(33, 2, "QOS: Parent Peer hit with hier code=" << hier->code <<
", TOS=" << (int)tos);
} else if (Ip::Qos::TheConfig.preserve_miss_tos &&
Ip::Qos::TheConfig.preserve_miss_tos_mask) {
tos = fd_table[fd].upstreamTOS &
Ip::Qos::TheConfig.preserve_miss_tos_mask;
debugs(33, 2, "QOS: Preserving TOS on miss, TOS=" << int(tos));
}
return setSockTos(fd, tos);
}
Thanks,
Andy
Re: [MERGE] Initial netfilter mark patch for comment
On Fri, 2010-08-20 at 12:44 -0600, Alex Rousskov wrote: > On 08/20/2010 11:06 AM, Andrew Beverley wrote: > > On Fri, 2010-08-13 at 18:19 -0600, Alex Rousskov wrote: > >> On 08/11/2010 03:25 PM, Andrew Beverley wrote: > >> > >>> I've moved these, as well as most of the other QOS functions, into > >>> Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem > >>> to fit with all these additional functions. > >> > >> * A patch preamble with the proposed commit message would be nice. > >> > >> * I am not sure what Qos class is. It is not documented. If it is a "QOS > >> configuration" class, I understand why we have a global instance of it, > >> but the original QosConfig name seems better in that case. If it is not > >> a configuration class, then I am not sure why we have a global instance > >> of it. And the QosConfig file name does not seem to match the class name > >> any more. > >> > >> Perhaps we need two classes, one for configuration and one for > >> manipulation? Or a Qos namespace with a configuration class and global > >> manipulation functions? The latter seems more likely. > >> > > > > I was trying to copy the approach used by the Interceptor class, which > > has its configuration variables and methods in one class, with a global > > instance of it. I thought that it would keep things nice and tidy by > > having all qos aspects in one single class (with the configuration > > variables within private space). > > Trust me, I feel your pain. Copying existing code on its own does not > guarantee much because a lot of code is broken, unfortunately. [ I am > not saying Interceptor class is or is not broken because I have not > reviewed it. ] > > > > I'm happy to go with whatever approach you want though, and can separate > > the config aspects back out from the functions. > > > > Before I go ahead and do this, I want to check which option I should go > > for. As Alex says, the options are: > > > > 1) > > - Reinstate the QosConfig class with the configuration variables as > > public members > > - Create a new class (QosFunctions?) with the relevant functions in > > - Create one global instance of each class, > > Rule of thumb: If you can use a namespace instead of a class, use a > namespace. For example, classes that only group related stateless > functions are usually a bad idea. > > > > or alternatively create a > > new instance of the functions class each time the functions need to be > > accessed (is the latter inefficient?) > > It is difficult for me to imagine a worse design (one class/instance per > function, perhaps?), but I am sure you can find it in Squid :-). > > > > 2) > > - Reinstate the QosConfig class with the configuration variables as > > public members > > - Create the functions as global functions in the namespace > > This is the best option if functions do not share or keep state. > > Please note that QosConfig should become Qos::Config once you add Qos > namespace. > > > > 3) > > - Keep everything in one class :-) > > > > > > If going with option 2, the functions can no longer be const (which most > > of them currently are). > > Well, if your methods do not share or keep state, they should not be > const. They should be static! Const means "I use but do not modify the > state of my object". And if a class has nothing but static methods, it > should be a namespace in most cases... > > > In summary, > > * Work inside Qos namespace. > > * Keep configuration state (data) inside Qos::Config class. You can use > a single global Qos::TheConfig "current configuration" instance of that > class if needed. > > * Functions that exist to interpret or modify configuration should be > Qos::Config methods (const if possible). > > * Functions that wrap library calls or otherwise manipulate some > external state/data that they do not and cannot own should be declared > outside Qos::Config and inside Qos. > > * If there is a non-configuration state/data that you can encapsulate > and manipulate in a class setting, then create more Qos::* classes. Thanks Alex. An excellent explanation. I fully understand now, and have gone with option 2 as you suggest, just keeping the isActive functions in the config class. Updated patch to follow soon. Andy
Re: [MERGE] Initial netfilter mark patch for comment
On Fri, 2010-08-13 at 18:19 -0600, Alex Rousskov wrote: > On 08/11/2010 03:25 PM, Andrew Beverley wrote: > > > I've moved these, as well as most of the other QOS functions, into > > Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem > > to fit with all these additional functions. > > * A patch preamble with the proposed commit message would be nice. > > * I am not sure what Qos class is. It is not documented. If it is a "QOS > configuration" class, I understand why we have a global instance of it, > but the original QosConfig name seems better in that case. If it is not > a configuration class, then I am not sure why we have a global instance > of it. And the QosConfig file name does not seem to match the class name > any more. > > Perhaps we need two classes, one for configuration and one for > manipulation? Or a Qos namespace with a configuration class and global > manipulation functions? The latter seems more likely. > I was trying to copy the approach used by the Interceptor class, which has its configuration variables and methods in one class, with a global instance of it. I thought that it would keep things nice and tidy by having all qos aspects in one single class (with the configuration variables within private space). I'm happy to go with whatever approach you want though, and can separate the config aspects back out from the functions. Before I go ahead and do this, I want to check which option I should go for. As Alex says, the options are: 1) - Reinstate the QosConfig class with the configuration variables as public members - Create a new class (QosFunctions?) with the relevant functions in - Create one global instance of each class, or alternatively create a new instance of the functions class each time the functions need to be accessed (is the latter inefficient?) 2) - Reinstate the QosConfig class with the configuration variables as public members - Create the functions as global functions in the namespace 3) - Keep everything in one class :-) If going with option 2, the functions can no longer be const (which most of them currently are). Thanks, Andy
Re: [MERGE] Initial netfilter mark patch for comment
> Seems the netfilter guys found a major problem with strtoul(). > Thankfully the same fix should work for us as well. > > > Luciano Coelho wrote: > > > > > Not easily. I found that there is a bug in strtoul (and strtoull for > > that matter) that causes the long to overflow if there are valid digits > > after the maximum possible digits for the base. For example if you try > > to strtoul 0xf (with 9 f's) the strtoul will overflow and come > > up with a bogus result. I can't easily truncate the string to avoid > > this problem, because with decimal or octal, the same valid value would > > take more spaces. I could do some magic here, checking whether it's a > > hex, dec or oct and truncate appropriately, but that would be very ugly. > > > > So the simplest way I came up with was to use strtoull and return > > -EINVAL if the value exceeds 32 bits. ;) > > Thanks for that Amos. I have to admit that when I was doing my initial browse of the iptables mark module, I noticed that they had their own xtables_strtoui function which limited the size. I'll check this out a bit more thoroughly. I'm away at the moment with minimal internet access, but I hope to pick up again on the patch shortly. Cheers, Andy
Re: Autoconf-refactor bugs?
> > Firstly, I'm slightly confused as to whether I am seeing the > > autoconf-refactor work in my copy of trunk (I have run bzr update). I > > didn't think I'd seen it go in, but delving into configure.in I think it > > might be in there... what should I look for to check whether it's > > definitely what I'm looking at? > > It is, it was merged two days ago. Ah yes, I've just realised that I misread Amos's "2 days ago" as "2 days to go"... > And you're right, it does have a few bugs. > Sorry about those. > > > Secondly, assuming it is in, there appear to be 2 small problems: > > > > 1. --disable-inline doesn't work for me. On investigation, this seems to > > be because enable_inline is set to "yes" at line 296, which overrides > > the later AC_ARG_ENABLE(inline), regardless of the setting. Also, I had > > to add in enable_inline=$enableval after the AC_ARG_ENABLE check (once > > I'd removed the enable_inline=yes) to get it to work. > > Fixed, in a slightly different way. > > > 2. Line 1344 (if "$squid_host_os" = "solaris" ; then) appears to be > > missing a 'test'. > > Fixed. > > > Sorry if I've jumped the gun and am actually looking at old code. > > You have not, and thank you for checking things out. > There is one more bug, related to the default hosts file and default > http and icp ports handling. > I've also fixed those. Great, thanks. I shall work on adding in the --with-netfilter-conntrack stuff :-) Unless you tell me otherwise, I plan on keeping it where I had it previously (around about line 1115 after SSL checks), but changing it as Amos said, so that it adds in the functionality if the libraries are present unless disabled. Andy
Re: comm_set_tos: setsockopt(IP_TOS) errors in Mac OSX 10.6.4
Andrew, > I have compile squid for the Mac OSX 10.6.4 and is working with no issue > with reguards too squid.conf setting , I run Privoxy --> Squid --> > tor=internet but when I look at the cache.log it is filling up with errors. > This seem too have happened on on the openbsd some time ago and never found > a solution. Is there a solution to this error ? > > Here is some of the sample errors > > > 2010/08/14 03:31:50| comm_set_tos: setsockopt(IP_TOS) on FD 23: (22) Invalid > argument > 2010/08/14 03:31:50| comm_set_tos: setsockopt(IP_TOS) on FD 22: (22) Invalid > argument Are you actually using the QOS functionality? If not, recompile without it (don't add --enable-zph-qos). If you are, then I saw this error when I was doing some recent development work. I think in my case it was caused by passing setsockopt invalid values. Do you have a valid value set for qos_flows in squid.conf? I'm currently rewriting a lot of the QOS functionality. If you are brave and otherwise having no luck, maybe you would like to try my latest patch. There's a recent copy in the squid-dev archives, although you will have to patch against a very recent version of the main code: http://www.squid-cache.org/mail-archive/squid-dev/201008/att-0101/netfilter-qos-20100811.patch Regards, Andy
Autoconf-refactor bugs?
Hi, Firstly, I'm slightly confused as to whether I am seeing the autoconf-refactor work in my copy of trunk (I have run bzr update). I didn't think I'd seen it go in, but delving into configure.in I think it might be in there... what should I look for to check whether it's definitely what I'm looking at? Secondly, assuming it is in, there appear to be 2 small problems: 1. --disable-inline doesn't work for me. On investigation, this seems to be because enable_inline is set to "yes" at line 296, which overrides the later AC_ARG_ENABLE(inline), regardless of the setting. Also, I had to add in enable_inline=$enableval after the AC_ARG_ENABLE check (once I'd removed the enable_inline=yes) to get it to work. 2. Line 1344 (if "$squid_host_os" = "solaris" ; then) appears to be missing a 'test'. Sorry if I've jumped the gun and am actually looking at old code. Regards, Andy
Re: [MERGE] Initial netfilter mark patch for comment
> >> stub* are cut down set of all non-inline Ip::QosConfig methods and any > >> globals defined in QosConfig.h. Changes to the API need to be mirrored > >> there. The functions inside usually call fatal() to alert a wrong > >> linkage clearly during testing. In this particular case the parse > >> function will need to be duplicated there. > > > > Sorry, I need some clarification on this. I've looked at most of the > > other stub files, and most of the functions do indeed just return > > fatal(). However, the existing stubQosConfig.cc is almost identical to > > QosConfig.cc, with almost all of its code. Shall I change the functions > > to fatal()? > > > > Presumably I should add all the new functions into it (getTosLocalMiss, > > getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS, > > getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)? > > It does need another looking at and possibly stripping down. IIRC it was > there from when QosConfig was a member of SquidConfig. I'll run a quick > check myself now to see what breaks in the current usage if the currently > defined stubs are stripped down. > > You will need to add the appropriate dead-end stubs for the new functions. Okay, I'll work on a stripped down fatal() for the time being. > > I've moved these, as well as most of the other QOS functions, into > > Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem > > to fit with all these additional functions. > > We are moving the rest of Squid in general towards a model of > Namespace::TheConfig containing the config data values pulled from > squid.conf separate from the code which utilizes them. > > Please wait for Alex as well on this particular change. I did consider having the config values in a separate Config class, and I'm happy to change to this, I just thought that it was neater having them in the same class. It also means that the configuration values do not need to be public, which I thought was generally a Good Thing. > My thoughts on it are these; > > I like the moving of functions to IP::Qos including the existing ones. > Under the coding guidelines the existence of class Qos calls for it to be > in a file src/ip/Qos.h and .cc. > Several of the functions particularly the is*Active() can be inlined for > better performance with _SQUID_INLINE_, which calls for a ip/Qos.cci file > to define them and be conditionally included in the .h or .cc based on #if > _USE_INLINE_. No problem. My initial thoughts are that I should do get*LocalMiss, get*LocalHit and is*Active (and possibly the new set functions). > Some set*Mark and set*Tos functions woudl round out the API. To take the > same parameters as the get*() ones, but which do the perform the > setsockopt. So that paired lines of: > unsigned int mark = Ip::Qos.getNfmarkLocalMiss(fd, > http->request->hier); > comm_set_nfmark(fd,mark); > become > Ip::Qos.setNfmarkLocalMiss(fd, http->request->hier); > > ... and thus comm.cc does not need anything about the MARK setsockopt. > Mirroring the way it has nothing to do with the MARK/TOS getsockopt etc. I like that; I'll make the changes. I guess there is then no need for getTosLocalHit() and getNfMarkLocalHit(), as all they do is return the appropriate private variables, although I could leave them there to keep everything consistent. > > > > I've carried out a fair bit of testing on the mark functionality today, > > and It Works For Me, but I'll be able to try it better in a production > > server in the coming weeks. > > > > Could you let me know if/what else I need to do before acceptance. I > > believe there are the following: > > > > - Confirm and implement stub function requirements > > - Factor the configure.in changes into Kinkie's autoconf-refactor? > > Those are the big ones. Along with getting the namespace change approved. > > The reconfigure straightening will change your dependency logic model a > bit. Specifically such that the absence of --with-netfilter-conntrack > (implicit / auto-detect case) results in the same auto-detect currently > only done by explicit "yes" results, but which does not terminate in > AC_MSG_ERROR. No problem. Any idea when the autoconf-refactor will appear in trunk? Andy
Re: [MERGE] Initial netfilter mark patch for comment
Updated patch attached; comments below.
> > If we can move to strtoul, I would like to change 'tos' to char
> > throughout. Currently it is possible to set it to invalid values in
> > squid.conf, which then cause problems with dumpConfigLine.
> >
> > Question number 2: what is stubQosConfig.cc? Does that also need
> > updating for this patch?
> >
>
> stub* are cut down set of all non-inline Ip::QosConfig methods and any
> globals defined in QosConfig.h. Changes to the API need to be mirrored
> there. The functions inside usually call fatal() to alert a wrong
> linkage clearly during testing. In this particular case the parse
> function will need to be duplicated there.
Sorry, I need some clarification on this. I've looked at most of the
other stub files, and most of the functions do indeed just return
fatal(). However, the existing stubQosConfig.cc is almost identical to
QosConfig.cc, with almost all of its code. Shall I change the functions
to fatal()?
Presumably I should add all the new functions into it (getTosLocalMiss,
getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS,
getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)?
> I have failed to find any problems with strtoul. Looks like it can be
> used. Although we may have to find a GPLv2 compatible implementation.
I have kept the strtoul parts, and changed (almost) all of the tos
variables to unsigned char.
>
> In this version the new methods look they should be in the Ip::Qos
> namespace.
>
> * the new clientReplyContext::tosLocalMissValue() and
> clientReplyContext::nfmarkLocalMissValue() methods particularly look
> like they really should take the hier code as a second parameter. Both
> fd and the hier if passed in should be const.
I've moved them to Ip::Qos and added the parameters as const.
> * the new FwdState getUpstream* methods are in a similar position but
> not quite so clear cut. If you can find way to cleanly move them there
> great, otherwise it's not so much a bit deal yet.
I've moved these, as well as most of the other QOS functions, into
Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem
to fit with all these additional functions.
> * Thank you for the extra documentation on comm_set_tos().
>
> * Please remove the bits touching comm_set_v6only() its 'tos' field is
> not related to QoS. Just acronym confusion.
My mistake, removed.
I've carried out a fair bit of testing on the mark functionality today,
and It Works For Me, but I'll be able to try it better in a production
server in the coming weeks.
Could you let me know if/what else I need to do before acceptance. I
believe there are the following:
- Confirm and implement stub function requirements
- Factor the configure.in changes into Kinkie's autoconf-refactor?
Regards,
Andy
=== modified file 'configure.in'
--- configure.in 2010-08-10 15:37:53 +
+++ configure.in 2010-08-11 13:32:45 +
@@ -1112,6 +1112,34 @@
fi
AC_SUBST(SSLLIB)
+dnl Allow user to specify libnetfilter_conntrack (needed for QOS netfilter marking)
+AC_ARG_WITH(netfilter-conntrack,
+ AS_HELP_STRING([--with-netfilter-conntrack=PATH],
+ [Compile with the Netfilter conntrack libraries. The path to
+ the development libraries and headers
+ installation can be specified if outside of the
+ system standard directories]), [
+case "$with_netfilter_conntrack" in
+ no)
+: # Nothing special to do here
+;;
+ yes)
+AC_CHECK_LIB([netfilter_conntrack], [nfct_query],,
+ AC_MSG_ERROR([libnetfilter-conntrack library not found. Needed for netfilter-conntrack support]),
+ [-lnetfilter_conntrack])
+AC_CHECK_HEADERS([libnetfilter_conntrack/libnetfilter_conntrack.h \
+ libnetfilter_conntrack/libnetfilter_conntrack_tcp.h])
+;;
+ *)
+if test ! -d $withval ; then
+ AC_MSG_ERROR([--with-netfilter-conntrack path does not point to a directory])
+fi
+LDFLAGS="-L$with_netfilter_conntrack/lib $LDFLAGS"
+CPPFLAGS="-I$with_netfilter_conntrack/include $CPPFLAGS"
+with_netfilter_conntrack=yes
+;;
+ esac
+])
AC_ARG_ENABLE(forw-via-db,
AS_HELP_STRING([--enable-forw-via-db],[Enable Forw/Via database]), [
@@ -1990,10 +2018,19 @@
SQUID_YESNO([$enableval],
[unrecognized argument to --enable-zph-qos: $enableval])
])
-SQUID_DEFINE_BOOL(USE_ZPH_QOS,${enable_zph_qos:=no},
+SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=no},
[Enable Zero Penalty Hit QOS. When set, Squid will alter the
TOS field of HIT responses to help policing network traffic])
AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
+if test "$enable_zph_qos" = "yes" ; then
+if test "$with_netfilter_conntrack" = "yes" ; then
+AC_MSG_NOTICE([QOS netfilter marking enabled: $with_netfilter_conntrack])
+SQUID_DEFINE_BOOL(USE_QOS_NFMARK,$with_netfilter_conntrack,
+
Re: [MERGE] Initial netfilter mark patch for comment
Thanks for the prompt response. Updated patch attached to my previous
email.
> > Please find attached the first version of the netfilter mark patch. I've
> > not yet tested it extensively, but would welcome some initial feedback
> > or comments.
>
> The mess around local port can be cleaned up (see comments below). The
> remote ip:port mess seems as clean as it can be in the current code.
>
> >
> > Is it too late to get this in v3.2? I will update the appropriate
> > release-notes once I know.
>
> Not too late. This can come under a header of polish on the existing QoS
> feature. We have a a bit more flexibility on that kind of change.
>
> The changes to qos_flows needs to be mentioned in section 3.2 "changes to
> existing tags" and the --with option to section 4.2 "new ./configure
> options". Both are alphabetical lists.
Release notes for 3.2 updated.
> configure.in: chunk line 2087
> * "QOS netfilter marking enabled: $with_netfilter_conntrack" if ZPH QoS
> is enabled, but regardless of whether NFMARK was defined.
> The notice needs to be inside the top part of your inner if statement.
Fixed.
> * Please also add an
> AC_CHECK_HEADERS([libnetfilter_conntrack/libnetfilter_conntrack.h
> libnetfilter_conntrack/libnetfilter_conntrack_tcp.h])
> That way you can error-out early in the ./configure build process with a
> useful AC_MSG_ERROR() and also wrap the headers in forward.cc according to
> squid coding style.
> NP: this check is also one that can be cached AC_CHECK_CACHED() I think.
Fixed. I've not looked at AC_CHECK_CACHED yet; I can do so, although it
doesn't appear to be used anywhere else in configure.in.
> src/cf.data.pre typo:
> "in the case of TOS preservation require your kernel to be patch with"
> ==> s /patch/patched/
Fixed.
> src/comm.cc:
> * Please use DBG_IMPORTANT instead of '1' and '0' in debugs. Also, IMO
> both should indicate "WARNING:", unless the one currently logged at level 1
> is common and trivial, in which case it should be dropped to level 2+.
Most warnings have been changed to 2, except where a higher level is
warranted.
> src/forward.cc:
> * Wrap system includes in "#if HAVE_*"
> ** also they appear to be duplicate included in forward.h and
> forward.cc, please only include in the forward.h.
Fixed.
> * Please remove the "defined(_SQUID_LINUX_)" around getNfctMark. We build
> on non-linux systems with possible netfilter support (kFreeBSD and hurd).
Fixed.
> * our source format requires function result type to be on a separate
> line in .cc:
> +int
> +FwdState::getNfctMark
Fixed.
> * also related to the above. The old ZPH TOS _SQUID_LINUX_ wrapper was
> only there due to the preserve-miss patch being linux kernel specific. I
> think your new preserve for marks not depending on kernel patching is
> likely not to need it, so the wrapping there can be re-worked a bit to only
> depend on USE_ZPH_QOS and USE_QOS_NFMARK. If you make this change the docs
> will also need updating.
The #defs have all been updated in accordance with my previous email.
> * You run the sequence:
> +serv_fde_local_conn.InitAddrInfo(addr);
> +serv_fde_local_conn.FreeAddrInfo(addr);
> +serv_fde_local_conn.GetAddrInfo(addr);
>
> ** The GetAddrInfo allocates dynamic memory and needs to be paired with a
> FreeAddrInfo() of its own.
> ** The structures created locally by Init inside addr should be able to
> be used throughout the sequence. Unless there is some garbage produced by
> getaddrinfo() please remove the Get and move the Free down below where you
> are finished with the addr variable and it's content:
>
> +serv_fde_local_conn.FreeAddrInfo(addr);
> +} else {
> +debugs(17, 1, "Failed to allocate new conntrack");
>
I tried removing the GetAddrInfo(), but that doesn't work as it
retrieves some of the required connection info. However, I have moved
the FreeAddrInfo down as suggested, but outside of the 'if' so that it
pairs with the GetAddrInfo.
> * following nfct_query() you can output serv_fde_local_conn directly in
> the debugs() argument instead of fiddling with inet_ntop to find its
> content. That will handle all the display mess and output src ip:port as a
> twinned pair. If the debugs result shows no port it means there was none
> set.
Fixed.
> * again with the debugs level s/1/DBG_IMPORTANT/ and tagged "WARNING:" if
> it's an error important message. Or bump to level 2+ if its not.
Fixed.
> src/ip/QosConfig.cc:
> * Ip::Qos::QosConfig::parseConfigLine again with the s/0/DBG_CRITICAL/.
> This one I think will screw up TOS settings if it continues so a "FATAL:"
> message and self_destruct() are warranted when mark configuration is
> attempted.
I've used DBG_CRITICAL throughout the parseConfigLine, as nothing else
produces any output. Let me know if I should change this.
> * Thanks for knowing about strtoul(). We had some issues with portability
> and the optional-prefixes last time we tried
Re: [MERGE] Initial netfilter mark patch for comment
On Mon, 2010-08-02 at 12:03 -0600, Alex Rousskov wrote: > On 08/01/2010 05:47 PM, Andrew Beverley wrote: > > Please find attached the first version of the netfilter mark patch. I've > > not yet tested it extensively, but would welcome some initial feedback > > or comments. Thanks for the prompt reply. Please find attached an updated version of the patch, which includes fixes to all the feedback below. It remains not-extensively tested, but is attached for further comments. > * It would be nice to get a proposed commit message describing the > changes as a patch preamble. Among other things, this will allow > reviewers to understand the overall scope and intent of your work. Sorry about that, this is new to me. As you've probably gathered, the aim of this patch is to implement the existing TOS functionality, but as netfilter marking. > * comm_set_mark() has a very generic name. Many things can be "marked", > for many reasons. Can you come up with a better, more specific one? Renamed to comm_set_nfmark. > * comm_set_mark() is not documented but is a part of the public Comm API. > Now documented in the source code. Is that the only place to document? > * getNfctMark() definition #ifdef conditions are inconsistent with its > declaration and use #ifdefs. Fixed. > * getNfctMark() is static but does not start with a capital letter. Fixed. > * getNfctMark() might belong to fde rather than FwdState because it does > not use FwdState. I am not sure about this one, but it may indicate a > general design problem -- a callback with no connection to the caller. I thought about moving to fde, but I feel it sits better in FwdState as although it is not called directly within it, it is called as a result of other code in there. getNfctMark() has been renamed to GetNfMarkCallback > * Please document who calls getNfctMark() and when. > Comments added. > * If getNfctMark() is an async callback that will be called some time > after the nfct_callback_register returns, how do you know that clientFde > is still a valid pointer _and_ is pointing to the same connection > information? > > * If getNfctMark() is an async callback that will be called at some > random time after the nfct_callback_register returns, is it safe to use > debugs()? I'm pretty sure it's synchronous: if I add a 3 second delay in the callback, then the rest of the code isn't executed until the callback has finished, and the conntrack information is still found. > * Please use Doxygen /** or /// comments for method descriptions. > Fixed. > * Please declare local variables when you first use them, if possible. > For example, Fixed. > * Please add a comment why ct = nfct_new() is not leaking memory despite > no matching delete/free OR fix the leak. nfct_destory() added. Thanks for pointing this out. > * upstreamMark member documentation should be fixed. What does that > member store/mean? I understand that you were copying the documentation > bug from upstreamTOS, but I hope we do not have to replicate that. upstreamMark and upstreamTOS fixed. > * Please break out new code into a new FwdState method(s) instead of > making FwdState::dispatch longer and uglier. I have added 2 new methods: getUpstreamTOS() and getUpstreamNfMark() > * Same comment applies to src/client_side_reply.cc code, including the > old QOS code already there. It should be extracted from regular methods > as they are getting difficult to follow, especially with all the ifdefs. I have added tosLocalMissValue() and nfmarkLocalMissValue() > * The new code implements a non-critical feature because errors do not > terminate transactions. Yet, most errors are reported at level 1 to > cache.log. We often have to modify the code to remove excessive > cache.log pollution because it hurts busy proxies. Do you need to use > level-1 reporting? Of every error? Or perhaps the code should just > increment some stats counters? I have moved most of the general errors to level 2. > * Is there a way to defer most support checks to runtime (like > comm_set_mark does it), to minimize the use of #ifdefs in the core code? > For example, can we use one #ifdef variable for both USE_QOS_NFMARK and > USE_ZPH_QOS code, in most contexts? They are intermixed in the code in a > complex ways that I find difficult to follow. I have simplified this and removed as many as possible. I have added isTosActive() and isMarkActive() as suggested by Amos, and used these instead. Some of the code relies on the libnetfilter_conntrack libraries, so I have had to keep that inside #ifdefs. This leads me on to my first question: why not just remove USE_ZPH_QOS and the compilation option --enable-zph-qos? With the attached patch, the code only runs when specified in squid.conf
[MERGE] Initial netfilter mark patch for comment
Please find attached the first version of the netfilter mark patch. I've not yet tested it extensively, but would welcome some initial feedback or comments. My comments are: - The existing TOS patch cannot be disabled at runtime. As such, this mark patch cannot be either. Would it be preferable to only enable them both if the qos_flows config option is present? This would also have the advantage of being able to add CAP_NET_ADMIN as appropriate at runtime. - I have used sscanf instead of strtoul for both TOS and MARK in QosConfig.cc (sscanf doesn't auto-detect the format of unsigned long int). As a result, the tos variable could be changed to type char, which is what it should be in my opinion. Should I do this? - The code in forward.cc to obtain all the data needed to locate the connection information is messy. Is there a better way to achieve it? Conntrack needs to be passed local and remote port numbers and IP addresses. Is it too late to get this in v3.2? I will update the appropriate release-notes once I know. Thanks, Andy # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: a...@andybev.com-20100801233324-uoy4sp2e3uszw5qn # target_branch: file:///home/andrew/squid-repo/trunk/ # testament_sha1: 4bd1e0b517dc3322fa114f30fb26ff2483cb3410 # timestamp: 2010-08-02 00:34:42 +0100 # base_revision_id: squ...@treenet.co.nz-20100801134158-\ # vhc003yv3ikwao7e # # Begin patch === modified file 'configure.in' --- configure.in 2010-08-01 06:29:48 + +++ configure.in 2010-08-01 20:09:13 + @@ -1146,6 +1146,32 @@ fi AC_SUBST(SSLLIB) +dnl Allow user to specify libnetfilter_conntrack (needed for QOS netfilter marking) +AC_ARG_WITH(netfilter-conntrack, + AS_HELP_STRING([--with-netfilter-conntrack=PATH], + [Compile with the Netfilter conntrack libraries. The path to + the development libraries and headers + installation can be specified if outside of the + system standard directories]), [ +case "$with_netfilter_conntrack" in + no) +: # Nothing special to do here +;; + yes) +AC_CHECK_LIB([netfilter_conntrack], [nfct_query],, + AC_MSG_ERROR([libnetfilter-conntrack library not found. Needed for netfilter-conntrack support]), + [-lnetfilter_conntrack]) +;; + *) +if test ! -d $withval ; then + AC_MSG_ERROR([--with-netfilter-conntrack path does not point to a directory]) +fi +LDFLAGS="-L$with_netfilter_conntrack/lib $LDFLAGS" +CPPFLAGS="-I$with_netfilter_conntrack/include $CPPFLAGS" +with_netfilter_conntrack=yes +;; + esac +]) AC_ARG_ENABLE(forw-via-db, AS_HELP_STRING([--enable-forw-via-db],[Enable Forw/Via database]), [ @@ -2061,6 +2087,15 @@ [Enable Zero Penalty Hit QOS. When set, Squid will alter the TOS field of HIT responses to help policing network traffic]) AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos]) +if test "$enable_zph_qos" = "yes" ; then +if test "$with_netfilter_conntrack" = "yes" ; then +SQUID_DEFINE_BOOL(USE_QOS_NFMARK,$with_netfilter_conntrack, + [Enable support for QOS netfilter packet marking]) +else +AC_MSG_WARN([--with-netfilter-conntrack not enabled. QOS features will not support Netfilter marking.]) +fi +AC_MSG_NOTICE([QOS netfilter marking enabled: $with_netfilter_conntrack]) +fi dnl --with-maxfd present for compatibility with Squid-2. dnl undocumented in ./configure --help to encourage using the Squid-3 directive. === modified file 'src/cf.data.pre' --- src/cf.data.pre 2010-07-29 13:04:44 + +++ src/cf.data.pre 2010-08-01 20:09:13 + @@ -1532,18 +1532,23 @@ LOC: Ip::Qos::TheConfig DOC_START Allows you to select a TOS/DSCP value to mark outgoing - connections with, based on where the reply was sourced. + connections with, based on where the reply was sourced. For + platforms using netfilter, allows you to set a netfilter mark + value instead of, or in addition to, a TOS value. TOS values really only have local significance - so you should know what you're specifying. For more information, see RFC2474, RFC2475, and RFC3260. The TOS/DSCP byte must be exactly that - octet value 0x00-0xFF. - Note that in practice often only values up to 0x3F are usable - as the two highest bits have been redefined for use by ECN - (RFC3168). - - This setting is configured by setting the source TOS values: + Note that in practice often only values up to 0x3F are usable as + the two highest bits have been redefined for use by ECN (RFC3168). + +Mark values can be any unsigned integer value (normally up to 0x) + + This setting is configured by setting the following values: + +tos|markWhether to set TOS or netfilter mark values local-hit=0xFF Value to mark local cache hits. @@ -1551,23 +1556,26 @@ parent-hit=0xFF Value to mark hits from parent peers. - - NOTE: 'miss' preserve featu
Re: [MERGE] Rename enable-linux-netfilter to enable-nf-transparent
> I'm not sure its fully worth doing this. > > * the "transparent" options are all due for a naming upgrade or > removal in the next major release anyway. Okay, I'm happy to leave as is. However, I would still suggest a review of the naming in the upgrade (see below). > > * linux-netfilter in fact enables both NAT (intercept) and TPROXY > (transparent) capture methods. And is documented so far as applying to > all supported netfilter targets. So naming for one specific of the two > (or three now that MARK is being added) is not reducing the confusion. Correct, I overlooked the NAT aspect, so probably best leaving it for the time being. I've added the configure option --with-netfilter-conntrack for the MARK patch, but maybe to make things clearer the options should be more like: --with-linux-netfilter and --with-linux-netfilter-conntrack This makes it a bit clearer that the two are related but separate. Thoughts? [[ Sorry, I'm still not on this mailing list and thus unable to reply to list emails. I've tried to subscribe twice in the last fortnight and have also emailed squid-dev-ow...@squid-cache.org, all with no luck. Could somebody add me please? In the interim, please copy me on posts, as I am currently using the web archives. ]] Thanks, Andy
[MERGE] Rename enable-linux-netfilter to enable-nf-transparent
I'd like to propose the attached patch, to rename the build option
--enable-linux-netfilter to --enable-nf-transparent. This is for 2
reasons:
1. It is consistent with the remainder of the transparent proxy options
(ifpw-transparent, ipf-transparent, pf-transparent).
2. It causes less confusion with my proposed netfilter marking patch,
which also relies on netfilter libraries, but different ones.
--enable-linux-netfilter implies the whole of the netfilter libraries
are being included, when in actual fact it is only one for the purposes
of transparent proxying.
Netfilter marking patch to follow soon...
Regards,
Andy
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: a...@andybev.com-20100731220533-vfdiehk6tplxcpio
# target_branch: file:///home/andrew/squid-repo/trunk/
# testament_sha1: feb94d9d6fa4acfcb0d195c816049f70d0c466a6
# timestamp: 2010-07-31 23:05:40 +0100
# base_revision_id: squ...@treenet.co.nz-20100731141830-\
# 60bm8quxdd78f5rz
#
# Begin patch
=== modified file 'configure.in'
--- configure.in 2010-07-31 14:18:30 +
+++ configure.in 2010-07-31 22:05:33 +
@@ -1302,14 +1302,19 @@
#will be AC_DEFINEd later, after checking for appropriate infrastructure
AC_MSG_NOTICE([PF-based transparent proxying requested: ${enable_pf_transparent:=auto}])
+# Tell people the enable-linux-netfilter option has been renamed
+AC_ARG_ENABLE(linux-netfilter, , [
+ AC_MSG_ERROR(--enable-linux-netfilter has been renamed to --enable-nf-transparent.)
+])
+
# Linux Netfilter Transparent Proxy
-AC_ARG_ENABLE(linux-netfilter,
- AS_HELP_STRING([--enable-linux-netfilter],
+AC_ARG_ENABLE(nf-transparent,
+ AS_HELP_STRING([--enable-nf-transparent],
[Enable Transparent Proxy support for Linux (Netfilter)]), [
SQUID_YESNO([$enableval],
- [unrecognized argument to --enable-linux-netfilter: $enableval])
+ [unrecognized argument to --enable-nf-transparent: $enableval])
])
-AC_MSG_NOTICE([Linux Netfilter support requested: ${enable_linux_netfilter:=auto}])
+AC_MSG_NOTICE([Netfilter based transparent proxying requested: ${enable_nf_transparent:=auto}])
#will be AC_DEFINEd later, after checking for appropriate infrastructure
dnl Enable Large file support
@@ -3116,25 +3121,25 @@
SQUID_DEFINE_BOOL(PF_TRANSPARENT,$enable_pf_transparent,
[Enable support for PF-style transparent proxying])
-if test "$enable_linux_netfilter" != "no" ; then
+if test "$enable_nf_transparent" != "no" ; then
if test "$ac_cv_header_linux_netfilter_ipv4_h" = "yes"; then
-if test "$enable_linux_netfilter" = "auto" ; then
- enable_linux_netfilter=yes
+if test "$enable_nf_transparent" = "auto" ; then
+ enable_nf_transparent=yes
fi
else
-if test "$enable_linux_netfilter" = "auto" ; then
- enable_linux_netfilter=no
+if test "$enable_nf_transparent" = "auto" ; then
+ enable_nf_transparent=no
else
- AC_MSG_ERROR([Linux Netfilter support requested but needed headers not found])
+ AC_MSG_ERROR([Netfilter based transparent proxying requested but needed headers not found])
fi
fi
fi
-SQUID_DEFINE_BOOL(LINUX_NETFILTER,$enable_linux_netfilter,
+SQUID_DEFINE_BOOL(NF_TRANSPARENT,$enable_nf_transparent,
[Enable support for Transparent Proxy on Linux via Netfilter])
dnl Netfilter TPROXY depends on libcap but the NAT parts can still work.
-AC_MSG_NOTICE([Support for Netfilter-based interception proxy requested: $enable_linux_netfilter])
-if test "$enable_linux_netfilter" = "yes" && test "$use_libcap" != "yes" ; then
+AC_MSG_NOTICE([Support for Netfilter-based interception proxy requested: $enable_nf_transparent])
+if test "$enable_nf_transparent" = "yes" && test "$use_libcap" != "yes" ; then
AC_MSG_WARN([Missing needed capabilities (libcap or libcap2) for TPROXY])
AC_MSG_WARN([Linux Transparent Proxy support WILL NOT be enabled])
AC_MSG_WARN([Reduced support to Interception Proxy])
=== modified file 'src/cf.data.pre'
--- src/cf.data.pre 2010-07-29 13:04:44 +
+++ src/cf.data.pre 2010-07-31 22:05:33 +
@@ -904,7 +904,7 @@
NAME: tproxy_uses_indirect_client
COMMENT: on|off
TYPE: onoff
-IFDEF: FOLLOW_X_FORWARDED_FOR&&LINUX_NETFILTER
+IFDEF: FOLLOW_X_FORWARDED_FOR&&NF_TRANSPARENT
DEFAULT: off
LOC: Config.onoff.tproxy_uses_indirect_client
DOC_START
=== modified file 'src/cf_gen_defines'
--- src/cf_gen_defines 2010-05-25 11:12:20 +
+++ src/cf_gen_defines 2010-07-31 22:05:33 +
@@ -9,7 +9,7 @@
define["FOLLOW_X_FORWARDED_FOR"]="--enable-follow-x-forwarded-for"
define["FOLLOW_X_FORWARDED_FOR&&DELAY_POOLS"]="--enable-follow-x-forwarded-for and --enable-delay-pools"
define["FOLLOW_X_FORWARDED_FOR&&ICAP_CLIENT"]="--enable-follow-x-forwarded-for and --enable-icap-client"
- define["FOLLOW_X_FORWARDED_FOR&&LINUX_NETFILTER"]="--enable-follow-x-forwarded-for and --enable-linux-netfilter"
+ define["FOLLOW_X_FORWARDED_FOR&&NF_TRANSPARENT"]="--enable-follow-x-forwarded-for and --enable-nf-transparent
[MERGE] Fixed missing test command when testing OS
Please find attached very minor patch for configure.in
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: a...@andybev.com-20100731150544-p9g5vwcakwomkug9
# target_branch: file:///home/andrew/squid-repo/trunk/
# testament_sha1: acdd91fe4f43f7f08ff31f224b583c5310f8398c
# timestamp: 2010-07-31 16:09:55 +0100
# base_revision_id: squ...@treenet.co.nz-20100731141830-\
# 60bm8quxdd78f5rz
#
# Begin patch
=== modified file 'configure.in'
--- configure.in 2010-07-31 14:18:30 +
+++ configure.in 2010-07-31 15:05:44 +
@@ -1377,7 +1377,7 @@
CXXFLAGS="`getconf ${buildmodel}_CFLAGS` $CXXFLAGS"
LIBS="`getconf ${buildmodel}_LIBS` $LIBS"
LDFLAGS="`getconf ${buildmodel}_LDFLAGS` $LDFLAGS"
- if "$squid_host_os" = "solaris" ; then
+ if test "$squid_host_os" = "solaris" ; then
# On Solaris getconf returns for CFLAGS -xarch=generic64, -Xa and -Usun options, and
# for LDFLAGS -xarch=generic64, but:
# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWVwW+3YAAWhfgAAwVGP//3MD
nAC/7//wUAM68OdOOl05BKJJgo8TTI1PKbSepmIJmiep6jNQ0CUSp+mCZAKfqmmgABKmRHqp
hPDRNPUamgAAwExAGAAASU0mE0AVHtKeTRPU9MoaHqPUYmmRomLHLYm26KFzD6MmECxn
vR628b8FfqyIhTrc0J0dQisY73X9vd98HLx3FUweiaLHK/socvjooqnRpUGQLoeP4MRSKwq06jyv
FpFoB/kti3hfSdVkyCLx264GYRnwENB53HWS8h4oZCH0g5TUwfB9BaniPr5m7hMQz7iUZPpliWLo
YK8PS2jxRkc8g48yJTln7dZpIRYTHFxoX1M7P0Jl+GoIhDVGhdEeGDY8RESkXzW8rk9LWqEMqLgi
OIKiY3iXdOp+STUhPWyQscC1vEZjhHENsFK2iScrGzNy/aDdOgSiShUqD6ZkI58tClsrYOWeI4sJ
ErB7BXgV0zVqLSVMX0w4RlgUfgSZdLexvMXsljMiZoawqS2GFgsTGmhpMmTpdJhviTnEVivjKzFq
2XMGsRXK0eA561OoXZofh1q0urp5akSYOtT0KiqvzE1i12c6rkcxu/GEb+bPLCgzpcwbJ2gp51ax
9+4GhLoIMmmkjFm7YonKJDc8m/gS22ldiJbuDLaHGcbY8YCdDvxYummZeaIb4qaV9R7Nv7ctdqOI
0kwSW0frxJBkVnqFXvJR72Et/ancsejCDXINV6I9qs8Hw3bJdneaBeEgNkssNi34sIhQeMEVPSHP
U3TTO2KuULUT28UMiUOOzuyBKarGyQpHiJOVe17iYxQXzFxRNWGgSz7vvanmGoutDwkmaPnuPYJ5
H3dwDnhV+XRmgFXSF5+w0ZbxMgTjs46QMDKH26lbG7ZKIth2A1k1PKhrhECEC2LnKoIwHh+2EqoA
5VKj0M5OXuDZmFuTJdYnEQ4MoG0JLgxK8l2d4mWaqmwNhVuCHGGmiPCr0uCxCGqZwRwcySQbRrQ7
wqx19k8VIteDHJb4OD9i9SSjUI0XlaD2fzHOsxR2OjXsoTRWvAHVOVuHIXqjdPonhkloVpkMJ3IW
NYtoWwC5fovEqBnUoudH/i7kinChILgt9uw=
Compilation flags for QOS netfilter mark patch
I'm currently editing configure.in for my proposed QOS mark patch. From a previous list message Amos suggested the following: >> --enable/disable-linux-netfilter will also be involved in the logics. If set to "no" then it override disables this netfilter feature. << I started to implement this, but it doesn't make much sense and could cause confusion. The 2 are independent (the mark patch does not depend on the netfilter_ipv4 header). Therefore, I think it would be more appropriate to rename this option to --enable-nf-transparent in line with the other transparent options (enable-ipfw-transparent, enable-ipf-transparent, enable-pf-transparent), as it only affects the transparent proxying. >> A new --with-netfilter-conntrack option will be needed to set linking for the particular library. With path as a optional parameter, and --without meaning not to link (probably to prevent the code building as well). << I agree with this. This should be the option to enable it (along with --enable-zph-qos). [[ Sorry I'm unable to reply to the original message, as I am not yet on the mailing list. Subscription request is with the moderator ]] Thanks, Andy
Re: Marking uncached packets with a netfilter mark value
> So, do you have a clear use-case we can add to the wiki and commit > message? I propose extending the current QualityOfService feature as follows. The existing http://wiki.squid-cache.org/Features/QualityOfService page should read: * Allows you to set a TOS/Diffserv value to mark local and peer hits. * For platforms using netfilter, allows you to set a netfilter mark value instead of, or in addition to, a TOS value. * Allows you to selectively set only sibling or parent requests * Allows any HTTP response towards clients to have the TOS value of the response coming from the remote server, or in the case of marking, the incoming connection's netfilter mark value. For this to work correctly with a TOS value, you will need to patch your linux kernel with the TOS preserving ZPH patch. The kernel patch can be downloaded from http://zph.bratcheda.org. No patch is needed for a netfilter mark. * Allows you to mask certain bits in the TOS or mark received, before copying the value towards clients. > qos_flows - adding an initial flag "tos"|"mark" which determines which > marking type is to be set. Followed by the current (or extended) > stream=value tags. Default to "tos" if missing for backward compatibility Agree with the above for the config file. > So we end up with: >qos_flows tos parent-hit=0xA sibling-hit=0xB >qos_flows mark local-miss=0x1 I propose just the addition of the tos|mark flag and leave the remainder of the options the same. I don't see any need to add a local-miss option, as the user can mark packets before they hit Squid. To keep things simple, I propose that the patch is still enabled with --enable-zph-qos as with the current TOS patch. However, the mark patch will need the libnetfilter_conntrack library, so should a separate compiler flag be used instead? Incidentally, there is a mistake in the documentation for the existing QOS patch. At http://www.squid-cache.org/Doc/config/qos_flows/ it states: disable-preserve-miss If set, any HTTP response towards clients will have the TOS value of the response comming from the remote server masked with the value of miss-mask. This should read: By default, the existing TOS value of the response coming from the remote server will be retained and masked with miss-mark. This option disables that feature. Regards, Andy
Re: Uncached packet marking patch
On Wed, 2010-07-14 at 23:35 +0100, Andrew Beverley wrote: > > > In order to obtain mark information from the existing connection (using > > > libnetfilter_conntrack), I need to know the local and remote port > > > number, and the local and remote IP address of each connection. Most of > > > this information is in 'class fde', but not all of it. Is it available > > > elsewhere, or will I need to retrieve it myself? If so, is using > > > getsockopt() the best way of doing so? > > > > In Squid-3.1+ the above info should all be stored in fde via Ip::Address. > > Which holds the port paired with address, unless erased by some update. > > I'm using comm_local_port() to get the local port, remoteAddr() for the > remote IP address, and remote_port for the remote port. However, I'm > struggling with the local IP address. If I use local_addr I just get > [::] (localhost?), but I need the internet facing local IP address (as > per the connection tracking table). Any clues please? All sorted. I'm using GetAddrInfo (passing getsockname the value of server_fd) for the local information and fde->ipaddr and fde->remote_port for the remote information. I'm successfully retrieving the mark on the incoming connection with these details. Thanks, Andy
Re: Uncached packet marking patch
> > In order to obtain mark information from the existing connection (using > > libnetfilter_conntrack), I need to know the local and remote port > > number, and the local and remote IP address of each connection. Most of > > this information is in 'class fde', but not all of it. Is it available > > elsewhere, or will I need to retrieve it myself? If so, is using > > getsockopt() the best way of doing so? > > In Squid-3.1+ the above info should all be stored in fde via Ip::Address. > Which holds the port paired with address, unless erased by some update. I'm using comm_local_port() to get the local port, remoteAddr() for the remote IP address, and remote_port for the remote port. However, I'm struggling with the local IP address. If I use local_addr I just get [::] (localhost?), but I need the internet facing local IP address (as per the connection tracking table). Any clues please? > Failing that the ConnectionDetails objects store it all (not available in > most of the code though). I'm currently working on comm upgrades to make > this info easily passed around, they are deadlined for "any time now". I'm happy to wait for that if it would be better. > > > > Can I be added to this mailing list for the short term until the patch > > is completed? > > Subscription is automated with a moderator signoff. I thought you already > went through that? Sorry, I thought I had to specifically request it first. I've sent the subscription request through. Thanks, Andy
Uncached packet marking patch
As per my previous posts, I'm working on a patch to implement the ZPH features of Squid, but with packet marking (use-case to follow once I'm sure I can achieve what I want to). In order to obtain mark information from the existing connection (using libnetfilter_conntrack), I need to know the local and remote port number, and the local and remote IP address of each connection. Most of this information is in 'class fde', but not all of it. Is it available elsewhere, or will I need to retrieve it myself? If so, is using getsockopt() the best way of doing so? Can I be added to this mailing list for the short term until the patch is completed? Thanks, Andy
Re: Marking uncached packets with a netfilter mark value
> > So, is the best way of implementing this to do the same as transparent > > proxying, and check whether the (proposed) marking option is enabled in > > squid.conf when executing restoreCapabilities? If the user has asked for > > packets to be marked, then CAP_NET_ADMIN will be retained. The mark > > would then be applied in comm.cc in a similar way to the TOS settings. > > > > Andy > > Cool. > So, do you have a clear use-case we can add to the wiki and commit > message? I'll send one through shortly (or should I add it myself?). Should it be the same as the items in the Features list? > What do you think, for the config UI: > qos_flows - adding an initial flag "tos"|"mark" which determines which > marking type is to be set. Followed by the current (or extended) > stream=value tags. Default to "tos" if missing for backward compatibility > So we end up with: >qos_flows tos parent-hit=0xA sibling-hit=0xB >qos_flows mark local-miss=0x1 I was thinking of a separate config option, but you're right, it makes sense to put this in the same option. > The current src/ip/QosConfig.h fields may become a sub-struct of fields > if there is a double-up in wanting to label a stream with both TOS and > mark. I can't see much requirement to do both, but I guess for completeness, as it's technically possible it should be implemented. I'd also like to implement a preserve-miss feature. However, in my initial testing I was unable to retrieve the mark on the packet received by Squid. Andy
Re: Marking uncached packets with a netfilter mark value
> > I have done some initial scoping, but have discovered that in order to > > mark a packet using setsockopt(), the process needs to be run as root. > > Are you sure it needs root and not just a suitable capability flag? From > what I can tel CAP_NET_ADMIN is sufficient. You're right, it only needs CAP_NET_ADMIN. I've just hacked tools.cc to add that capability and it worked. So, is the best way of implementing this to do the same as transparent proxying, and check whether the (proposed) marking option is enabled in squid.conf when executing restoreCapabilities? If the user has asked for packets to be marked, then CAP_NET_ADMIN will be retained. The mark would then be applied in comm.cc in a similar way to the TOS settings. Andy
Re: Marking uncached packets with a netfilter mark value
> > 1. Because the marking process needs to be run as root, can this only be > > achieved by putting the mark function within the squid process that > > originally starts up, and stipulate that this has to be run as root? > > Consider a dedicated helper like the diskd helper - send it a fd using > shm, and a mark to place, and have it make the call. This can be > started up before squid drops privileges. Better still, to a patch to > netfilter to allow non root capabilities here. How about using enter_suid() and leave_suid() before and after the marking (which someone on the netfilter list suggested)? I have just tried it now and it seems to work okay. My intention would be to add the marking function into comm.cc like the current QOS/TOS functions are (comm_set_tos). Thanks, Andy
Marking uncached packets with a netfilter mark value
I am considering writing a patch for Squid so that it maintains a packet's netfilter mark value if not fetched from the cache. This would be similar to the QOS functionality, in that there would also be an option to set the mark on a packet that is fetched from the cache. I have done some initial scoping, but have discovered that in order to mark a packet using setsockopt(), the process needs to be run as root. My questions therefore are: 1. Because the marking process needs to be run as root, can this only be achieved by putting the mark function within the squid process that originally starts up, and stipulate that this has to be run as root? 2. Is any such patch likely to be accepted? Thanks, Andy
