SSl-bump wiki entry outdated

[Amos Jeffries]

Alex, Christos - anybody else with good SSL-bump knowledge

Can somebody please udate http://wiki.squid-cache.org/Features/SslBump 
to document the options for bumping in 3.3 and recommend the safest 
possible bumping configuration.
It is currently still stuck on the 3.1 config examples and leading 
people to both warnings in 3.3 and unsafe configurations.


Cheers
Amos

Re: [PATCH] Sending root certificate for validation

[Amos Jeffries]

On 8/06/2013 4:20 a.m., Tsantilas Christos wrote:

This patch modify squid cert validation subsystem to sent to cert
validator helper the complete certificates chain, not only the
certificates sent by web server. This is may not be possible in all
cases, for example  in cases where the root certificate is not stored
locally.

This is a Measurement Factory project



in globals.h:
 * please do not add any new entries in globals.h - we are trying to 
remove things from there.


* can you please also use the wrapper types provided in 
src/ssl/gadgets.h. ie X509_STACK_Pointer for things like STACK_OF(X509) 
and avoiding use of raw pointers to OpenSSL internal types?


Amos

Re: [PATCH] Error page format codes upgrade

[Amos Jeffries]

I will apply this patch to trunk shortly unless there are an last minute 
objections.


Amos

Re: /bzr/squid3/trunk/ r12903: Instruct clang not to treat unused command line arguments as errors

[Kinkie]

On Mon, Jun 10, 2013 at 11:27 AM, Amos Jeffries  wrote:
> On 10/06/2013 8:35 a.m., Francesco Chemolli wrote:
>>
>> 
>> revno: 12903
>> committer: Francesco Chemolli 
>> branch nick: trunk
>> timestamp: Sun 2013-06-09 22:35:58 +0200
>> message:
>>Instruct clang not to treat unused command line arguments as errors
>> modified:
>>acinclude/compiler-flags.m4
>
>
> Anyone known why we have the "-Wno-error=parentheses-equality" option in teh
> first place?
>  It would seem to me to be one of the warnings highlighting a coding
> guideline violation we need to fix in the sources. Not something to be
> suppressed.

I do, as I added it:

in case of
if (bool foo = somefunction()) {
}

gcc -Werror barfs unless it's expressed as:
if ((bool foo = somefunction())) {
}

while clang -Werror barfs for the unneeded parentheses, at least for
some clang versions
That option makes clang accept the GCC-ism.
The portable solution would probably be a macro:
stuff like:
if (MAYBE_PARENTHESIZE(bool foo = somefunction()) {
}

That'd be a readability FAIL.

--
/kinkie

Re: /bzr/squid3/trunk/ r12903: Instruct clang not to treat unused command line arguments as errors

[Amos Jeffries]

On 11/06/2013 12:59 a.m., Kinkie wrote:

On Mon, Jun 10, 2013 at 11:27 AM, Amos Jeffries  wrote:

On 10/06/2013 8:35 a.m., Francesco Chemolli wrote:


revno: 12903
committer: Francesco Chemolli 
branch nick: trunk
timestamp: Sun 2013-06-09 22:35:58 +0200
message:
Instruct clang not to treat unused command line arguments as errors
modified:
acinclude/compiler-flags.m4


Anyone known why we have the "-Wno-error=parentheses-equality" option in teh
first place?
  It would seem to me to be one of the warnings highlighting a coding
guideline violation we need to fix in the sources. Not something to be
suppressed.

I do, as I added it:

in case of
if (bool foo = somefunction()) {
}

gcc -Werror barfs unless it's expressed as:
if ((bool foo = somefunction())) {
}

while clang -Werror barfs for the unneeded parentheses, at least for
some clang versions
That option makes clang accept the GCC-ism.
The portable solution would probably be a macro:
stuff like:
if (MAYBE_PARENTHESIZE(bool foo = somefunction()) {
}

That'd be a readability FAIL.


Or auto-detect what clang version in use requires the option to compile 
the test cases?
That would allow us to enable the checks as often as possible without 
causing problems on specific clang.


Also, note that we have a minimum version of clang simpy to be able to 
compile the stdc++11 features we use. Halting the build in ./configure 
with a message to upgrade clang is a possibility.


The cases here are that assignment "if ((a=b))" requires parenthesis, 
equality "if (a==b)" requires single parenthesis. Otherwise the compiler 
generated code is not completely predictable. GCC and clang should both 
complain only if the cases are mixed up, otherwise it is a bug in the 
compiler.


Amos

Build failed in Jenkins: 3.HEAD-amd64-FreeBSD-9.0-clang #271

[noc]

See 


Changes:

[Amos Jeffries] Bug 3722: Invalid markup in Armenian hy ERR_ONLY_IF_CACHED_MISS

--
[...truncated 17830 lines...]
mv -f .deps/Pages.Tpo .deps/Pages.Plo
/bin/sh ../../libtool  --tag=CXX   --mode=compile clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
  -I../../.. -I../../../include -I../../../lib  -I../../../src -I../../include  
-I/usr/local/include -I/usr/include  -I/usr/include -I../../../libltdl  
-I/usr/include  -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments  -D_REENTRANT -g -O2 -MT PageStack.lo -MD -MP -MF 
.deps/PageStack.Tpo -c -o PageStack.lo `test -f 'mem/PageStack.cc' || echo 
'../../../src/ipc/'`mem/PageStack.cc
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT PageStack.lo -MD -MP -MF 
.deps/PageStack.Tpo -c ../../../src/ipc/mem/PageStack.cc  -fPIC -DPIC -o 
.libs/PageStack.o
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT PageStack.lo -MD -MP -MF 
.deps/PageStack.Tpo -c ../../../src/ipc/mem/PageStack.cc  -fPIC -DPIC -o 
PageStack.o >/dev/null 2>&1
mv -f .deps/PageStack.Tpo .deps/PageStack.Plo
/bin/sh ../../libtool  --tag=CXX   --mode=compile clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
  -I../../.. -I../../../include -I../../../lib  -I../../../src -I../../include  
-I/usr/local/include -I/usr/include  -I/usr/include -I../../../libltdl  
-I/usr/include  -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments  -D_REENTRANT -g -O2 -MT Segment.lo -MD -MP -MF 
.deps/Segment.Tpo -c -o Segment.lo `test -f 'mem/Segment.cc' || echo 
'../../../src/ipc/'`mem/Segment.cc
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT Segment.lo -MD -MP -MF 
.deps/Segment.Tpo -c ../../../src/ipc/mem/Segment.cc  -fPIC -DPIC -o 
.libs/Segment.o
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT Segment.lo -MD -MP -MF 
.deps/Segment.Tpo -c ../../../src/ipc/mem/Segment.cc  -fPIC -DPIC -o Segment.o 
>/dev/null 2>&1
mv -f .deps/Segment.Tpo .deps/Segment.Plo
/bin/sh ../../libtool --tag=CXX--mode=link clang++ -Werror 
-Wno-error=parentheses-equality -Qunused-arguments  -D_REENTRANT -g -O2   -g 
-pthread -o libipc.la  AtomicWord.lo FdNotes.lo Kid.lo Kids.lo  Queue.lo 
ReadWriteLock.lo StartListening.lo StoreMap.lo  StrandCoord.lo StrandSearch.lo 
SharedListen.lo TypedMsgHdr.lo  Coordinator.lo UdsOp.lo Port.lo Strand.lo 
Forwarder.lo  Inquirer.lo Page.lo PagePool.lo Pages.lo PageStack.lo  Segment.lo 
 
libtool: link: /usr/bin/ar cru .libs/libipc.a .libs/AtomicWord.o 
.libs/FdNotes.o .libs/Kid.o .libs/Kids.o .libs/Queue.o .libs/ReadWriteLock.o 
.libs/StartListening.o .libs/StoreMap.o .libs/StrandCoord.o 
.libs/StrandSearch.o .libs/SharedListen.o .libs/TypedMsgHdr.o 
.libs/Coordinator.o .libs/UdsOp.o .libs/Port.o .libs/Strand.o .libs/Forwarder.o 
.libs/Inquirer.o .libs/Page.o .libs/PagePool.o .libs/Pages.o .libs/PageStack.o 
.libs/Segment.o 
libtool: link: ranlib .libs/libipc.a
libtool: link: ( cd ".libs" && rm -f "libipc.la" && ln -s "../libipc.la" 
"libipc.la" )
Making all in mgr
/b

Re: /bzr/squid3/trunk/ r12903: Instruct clang not to treat unused command line arguments as errors

[Alex Rousskov]

On 06/10/2013 03:27 AM, Amos Jeffries wrote:
> On 10/06/2013 8:35 a.m., Francesco Chemolli wrote:
>> 
>> revno: 12903
>> committer: Francesco Chemolli 
>> branch nick: trunk
>> timestamp: Sun 2013-06-09 22:35:58 +0200
>> message:
>>Instruct clang not to treat unused command line arguments as errors
>> modified:
>>acinclude/compiler-flags.m4
> 
> Anyone known why we have the "-Wno-error=parentheses-equality" option in
> teh first place?
>  It would seem to me to be one of the warnings highlighting a coding
> guideline violation we need to fix in the sources. Not something to be
> suppressed.

I cannot find the description of that clang warning option, but if it
focuses on extra parentheses in benign ((a == b)) cases, then it is a
very minor problem not an important violation. And, as Kinkie pointed
out, if it prevents us from righting (bool a = b), it should stay
disabled even if it finds true problems like (a = b). We can rely on GCC
to spot those true problems.


Cheers,

Alex.

Re: [PATCH] Sending root certificate for validation

[Tsantilas Christos]

On 06/10/2013 03:16 PM, Amos Jeffries wrote:
> On 8/06/2013 4:20 a.m., Tsantilas Christos wrote:
>> This patch modify squid cert validation subsystem to sent to cert
>> validator helper the complete certificates chain, not only the
>> certificates sent by web server. This is may not be possible in all
>> cases, for example  in cases where the root certificate is not stored
>> locally.
>>
>> This is a Measurement Factory project
>>
> 
> in globals.h:
>  * please do not add any new entries in globals.h - we are trying to
> remove things from there.

I am suggesting to leave it here for this patch, and then with a
separate patch remove all similar entries from global.h to ssl/support.h
file (or the opposite, move from global.h similar entries and then apply
this patch)

> 
> * can you please also use the wrapper types provided in
> src/ssl/gadgets.h. ie X509_STACK_Pointer for things like STACK_OF(X509)
> and avoiding use of raw pointers to OpenSSL internal types?

Amos, it will not help anywhere, and also my sense is that it will make
the code worst ...

> 
> Amos
> 

Re: [PATCH] Error page format codes upgrade

[Alex Rousskov]

On 06/02/2013 05:08 AM, Amos Jeffries wrote:
> On 7/04/2013 5:48 p.m., Amos Jeffries wrote:
>> What this does is convert ErrorState object to using the generic
>> libformat.la parser and macro expansions instead of its own rather
>> limited custom ones for error pages and deny_info URL creation.

Thank you for addressing many of my concerns.


> +ErrorState(err_type type, Http::StatusCode, HttpRequest * request, 
> AccessLogEntry *ale = NULL);

I do not see ErrorState constructor being called with an ALE object (the
last parameter) anywhere. FwdState, for example, stores ALE but does not
pass it to ErrorState. Should not it? Any other contexts where we would
be better off passing readily available ALE instead of relying on three
levels of volatile and obscure pointers to lead us to it?

I also wonder if it would be a good idea to make that new ale parameter
mandatory so that folks are reminded to supply ALE when their context
has one.


> +// if request exists, we might have access to the transaction ALE object
> +if (ale_ == NULL && request != NULL && 
> request->clientConnectionManager.valid() &&
> +request->clientConnectionManager->currentobject != NULL) {
> +ClientSocketContext::Pointer c = 
> request->clientConnectionManager->currentobject;
> +ale_ = c->http->al;
> +}

Please use ConnStateData::getCurrentContext() since we have that
accessor and this access is outside ConnStateData.

During pipelining, are we certain that currentobject will point to the
request we are creating the error page for??



>  /* - IP stuff */
> -str.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));
> +//char ntoabuf[MAX_IPSTRLEN];
> +//XXXstr.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));

Is this change intentional?



Please also add a commit (and release notes?) comment about the old
error pages changing if they contained unsupported macros, as we
discussed earlier when looking at the

> -case '%':
> -p = "%";
> -break;
> -
>  default:
> -mb.Printf("%%%c", token);
> -do_quote = 0;
>  break;
>  }

change.



>  if (type == LFT_NONE) {
> -fatalf("Can't parse configuration token: '%s'\n", def);
> +debugs(46, 3, "Can't parse configuration token: '" << 
> StringArea(def,min(strlen(def), static_cast(10))) << "'");
> +// NP: unknown token found. Assume it should have been %% instead of 
> %.
> +if (def[0] == '%') {
> +type = LFT_PERCENT;
> +++cur;
> +}

Why do we want to start hiding these errors now? Is hiding them related
to this patch scope? Should we at least warn the admin that their page
may be buggy (but, as you know, I personally prefer a fatal exit on
configuration errors).


Thank you,

Alex.

Re: [PATCH] Sending root certificate for validation

[Alex Rousskov]

On 06/10/2013 10:31 AM, Tsantilas Christos wrote:
> On 06/10/2013 03:16 PM, Amos Jeffries wrote:
>> On 8/06/2013 4:20 a.m., Tsantilas Christos wrote:
>>> This patch modify squid cert validation subsystem to sent to cert
>>> validator helper the complete certificates chain, not only the
>>> certificates sent by web server. This is may not be possible in all
>>> cases, for example  in cases where the root certificate is not stored
>>> locally.


>> in globals.h:
>>  * please do not add any new entries in globals.h - we are trying to
>> remove things from there.

> I am suggesting to leave it here for this patch, and then with a
> separate patch remove all similar entries from global.h to ssl/support.h
> file (or the opposite, move from global.h similar entries and then apply
> this patch)


If there are already similar entries in global.h, then I agree that the
Christos suggestion is the best way forward. It allows this change to
stay focused and consistent while also implementing a desirable cleanup.



>> * can you please also use the wrapper types provided in
>> src/ssl/gadgets.h. ie X509_STACK_Pointer for things like STACK_OF(X509)
>> and avoiding use of raw pointers to OpenSSL internal types?
> 
> Amos, it will not help anywhere, and also my sense is that it will make
> the code worst ...


Various SSL _Pointer types are needed to free unused SSL objects (they
are all TidyPointers) and also unlock some of the locked SSL objects
after use. They are _not_ appropriate for temporary no-storage access to
objects stored by OpenSSL because they do not add safety but will
accidentally _delete_ those internal objects if we are not very careful.

Amos, are any of your Pointer concerns related to objects that Christos
stores or creates? If not (i.e., all uses are temporary convenience
variables), we should not use TidyPointers for them.


Christos, the following code does _not_ create a new STACK_OF(X509)
object and does _not_ increment the lock count on an old object, right?

> +if (!peerCerts)
> +peerCerts = SSL_get_peer_cert_chain(vcert.ssl);
> +


Thank you,

Alex.

Re: Should we remove ESI?

[Alex Rousskov]

On 06/09/2013 02:40 PM, Kinkie wrote:

>   while attempting to increase portability to recent clang releases, I
> noticed that libTrie hasn't benefited from the portability work that
> was done in the past few years.
> 
> I can see three ways to move forward:
> 1- replicate these changes into libTrie
> 2- change libTrie to piggyback squid's configuration variables
> 3- fully integrate libTrie into squid's build system. Unless Robert
> knows otherwise, squid is the only user of this library..


I cannot tell what libTrie does: The README file is empty and the commit
message only implies that it is an ESI component. AFAICT, only ESI uses
it today.

I do not know much about ESI, but IMHO, if somebody has cycles to work
on this, it would be best to spend them removing ESI (together with
libtTrie) from Squid sources while converting ESI into an eCAP adapter.
This will be a big step forward towards making client side code sane
(but removing ESI itself does not require making complex changes to the
client side code itself).


I am willing to assist with the eCAP APIs necessary for the conversion,
but I cannot volunteer to work on ESI removal or the ESI adapter,
unfortunately. Perhaps somebody who uses ESI can help?


$0.02,

Alex.

Re: [PATCH] Sending root certificate for validation

[Tsantilas Christos]

On 06/10/2013 08:06 PM, Alex Rousskov wrote:
> On 06/10/2013 10:31 AM, Tsantilas Christos wrote:
>> On 06/10/2013 03:16 PM, Amos Jeffries wrote:
>>> On 8/06/2013 4:20 a.m., Tsantilas Christos wrote:
 This patch modify squid cert validation subsystem to sent to cert
 validator helper the complete certificates chain, not only the
 certificates sent by web server. This is may not be possible in all
 cases, for example  in cases where the root certificate is not stored
 locally.
> 
> 
>>> in globals.h:
>>>  * please do not add any new entries in globals.h - we are trying to
>>> remove things from there.
> 
>> I am suggesting to leave it here for this patch, and then with a
>> separate patch remove all similar entries from global.h to ssl/support.h
>> file (or the opposite, move from global.h similar entries and then apply
>> this patch)
> 
> 
> If there are already similar entries in global.h, then I agree that the
> Christos suggestion is the best way forward. It allows this change to
> stay focused and consistent while also implementing a desirable cleanup.

Yes there are a number of similar entries, the indexes of attached
objects to SSL.


> 
> 
> 
>>> * can you please also use the wrapper types provided in
>>> src/ssl/gadgets.h. ie X509_STACK_Pointer for things like STACK_OF(X509)
>>> and avoiding use of raw pointers to OpenSSL internal types?
>>
>> Amos, it will not help anywhere, and also my sense is that it will make
>> the code worst ...
> 
> 
> Various SSL _Pointer types are needed to free unused SSL objects (they
> are all TidyPointers) and also unlock some of the locked SSL objects
> after use. They are _not_ appropriate for temporary no-storage access to
> objects stored by OpenSSL because they do not add safety but will
> accidentally _delete_ those internal objects if we are not very careful.
> 
> Amos, are any of your Pointer concerns related to objects that Christos
> stores or creates? If not (i.e., all uses are temporary convenience
> variables), we should not use TidyPointers for them.
> 
> 
> Christos, the following code does _not_ create a new STACK_OF(X509)
> object and does _not_ increment the lock count on an old object, right?

Right.
The STACK_OF does not support locking.

> 
>> +if (!peerCerts)
>> +peerCerts = SSL_get_peer_cert_chain(vcert.ssl);
>> +
> 
> 
> Thank you,
> 
> Alex.
> 
> 

Re: [PATCH] Support forwarding intercepted but not bumped connections to cache_peers

[Alex Rousskov]

On 06/07/2013 10:45 AM, Alex Rousskov wrote:
> On 05/24/2013 05:58 PM, Alex Rousskov wrote:
>> When talking to a cache_peer (i.e., sending a CONNECT request before
>> tunneling the transaction), tunnel code is using a clever hack: Squid
>> does not parse the CONNECT response from peer but blindly forwards it to
>> the client. This works great and simplifies code a lot, except when the
>> client connection was intercepted and, hence, the client did not send a
>> CONNECT request and is not expecting a CONNECT response.
>>
>> In those situations, the patch accumulates, parses, and strips the peer
>> CONNECT response (or closes connection on errors).
>>
>> The existing tunnel I/O code is too simple to accommodate that task --
>> it cannot accumulate read data (its I/O buffers work in lockstep
>> fashion, writing everything it reads before reading again). Instead of
>> rewriting the entire tunnel code to use more complex buffers, I added a
>> temporary accumulation buffer for the CONNECT response. That buffer is
>> not allocated unless it is needed and does not grow beyond
>> SQUID_TCP_SO_RCVBUF size, just like the simple buffers.
> 
> I will commit this fix shortly unless there are last-minute objections.

Committed as trunk r12905.

Alex.

Build failed in Jenkins: 3.HEAD-amd64-opensuse #522

[noc]

See 

--
Started by upstream project "3.HEAD-amd64-CentOS-5.3" build number 2464
originally caused by:
 Started by an SCM change
Building remotely on opensuse-x64 in workspace 

$ bzr revision-info -d 

info result: bzr revision-info -d 
 returned 0. 
Command output: "12903 kin...@squid-cache.org-20130609203558-3u1r8742gqjs9tmg
" stderr: ""
[3.HEAD-amd64-opensuse] $ bzr pull --overwrite 
http://bzr.squid-cache.org/bzr/squid3/trunk/
bzr: ERROR: Connection error: while sending GET /bzr/squid3/trunk: [Errno 110] 
Connection timed out
ERROR: Failed to pull
Since BZR itself isn't crash safe, we'll clean the workspace so that on the 
next try we'll do a clean pull...
Retrying after 10 seconds
Cleaning workspace...
$ bzr branch http://bzr.squid-cache.org/bzr/squid3/trunk/ 

bzr: ERROR: Connection error: while sending POST /bzr/squid3/trunk/.bzr/smart: 
[Errno 110] Connection timed out
ERROR: Failed to branch http://bzr.squid-cache.org/bzr/squid3/trunk/
Retrying after 10 seconds
Cleaning workspace...
$ bzr branch http://bzr.squid-cache.org/bzr/squid3/trunk/ 

bzr: ERROR: Connection error: while sending POST /bzr/squid3/trunk/.bzr/smart: 
[Errno 110] Connection timed out
ERROR: Failed to branch http://bzr.squid-cache.org/bzr/squid3/trunk/

Build failed in Jenkins: 3.HEAD-amd64-FreeBSD-7.2 #1887

[noc]

See 

--
Started by upstream project "3.HEAD-amd64-CentOS-5.3" build number 2464
originally caused by:
 Started by an SCM change
Building remotely on east in workspace 

$ bzr revision-info -d 

info result: bzr revision-info -d 
 returned 0. 
Command output: "12903 kin...@squid-cache.org-20130609203558-3u1r8742gqjs9tmg
" stderr: ""
[3.HEAD-amd64-FreeBSD-7.2] $ bzr pull --overwrite 
http://bzr.squid-cache.org/bzr/squid3/trunk/
bzr: ERROR: Connection error: while sending GET /bzr/squid3/trunk: (60, 
'Operation timed out')
ERROR: Failed to pull
Since BZR itself isn't crash safe, we'll clean the workspace so that on the 
next try we'll do a clean pull...
Retrying after 10 seconds
Cleaning workspace...
$ bzr branch http://bzr.squid-cache.org/bzr/squid3/trunk/ 

bzr: ERROR: Connection error: while sending POST /bzr/squid3/trunk/.bzr/smart: 
(60, 'Operation timed out')
ERROR: Failed to branch http://bzr.squid-cache.org/bzr/squid3/trunk/
Retrying after 10 seconds
Cleaning workspace...
$ bzr branch http://bzr.squid-cache.org/bzr/squid3/trunk/ 

bzr: ERROR: Connection error: while sending POST /bzr/squid3/trunk/.bzr/smart: 
(60, 'Operation timed out')
ERROR: Failed to branch http://bzr.squid-cache.org/bzr/squid3/trunk/

Re: [PATCH] Error page format codes upgrade

[Amos Jeffries]

On 11/06/2013 4:53 a.m., Alex Rousskov wrote:

On 06/02/2013 05:08 AM, Amos Jeffries wrote:

On 7/04/2013 5:48 p.m., Amos Jeffries wrote:

What this does is convert ErrorState object to using the generic
libformat.la parser and macro expansions instead of its own rather
limited custom ones for error pages and deny_info URL creation.

Thank you for addressing many of my concerns.



+ErrorState(err_type type, Http::StatusCode, HttpRequest * request, 
AccessLogEntry *ale = NULL);

I do not see ErrorState constructor being called with an ALE object (the
last parameter) anywhere. FwdState, for example, stores ALE but does not
pass it to ErrorState. Should not it? Any other contexts where we would
be better off passing readily available ALE instead of relying on three
levels of volatile and obscure pointers to lead us to it?


The problem is whether ALE is filled out at the time it is passed to 
ErrorState. A lot of the code in server-side is depending on the ALE 
being filled out by logging, long after the error has been generated. I 
would rather use this slightly messy code that fails over to generating 
an ALE filled out with all the required details than suddenly make all 
the errors output by Squid have missing information.



I also wonder if it would be a good idea to make that new ale parameter
mandatory so that folks are reminded to supply ALE when their context
has one.


Maybe.  Im not sure we want to just pass it in without checking its 
up-to-date and that is a bit difficult with some of the current code.





+// if request exists, we might have access to the transaction ALE object
+if (ale_ == NULL && request != NULL && request->clientConnectionManager.valid() 
&&
+request->clientConnectionManager->currentobject != NULL) {
+ClientSocketContext::Pointer c = 
request->clientConnectionManager->currentobject;
+ale_ = c->http->al;
+}

Please use ConnStateData::getCurrentContext() since we have that
accessor and this access is outside ConnStateData.

During pipelining, are we certain that currentobject will point to the
request we are creating the error page for??


No. But that is no more certain when using the accessor in the current 
pipelining design.





  /* - IP stuff */
-str.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));
+//char ntoabuf[MAX_IPSTRLEN];
+//XXXstr.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));

Is this change intentional?


Yes with the src_addr removal we have no way to identify the client IP 
in that code anymore.




Please also add a commit (and release notes?) comment about the old
error pages changing if they contained unsupported macros, as we
discussed earlier when looking at the


There is no longer any change. see below..




-case '%':
-p = "%";
-break;
-
  default:
-mb.Printf("%%%c", token);
-do_quote = 0;
  break;
  }

change.




  if (type == LFT_NONE) {
-fatalf("Can't parse configuration token: '%s'\n", def);
+debugs(46, 3, "Can't parse configuration token: '" << StringArea(def,min(strlen(def), 
static_cast(10))) << "'");
+// NP: unknown token found. Assume it should have been %% instead of %.
+if (def[0] == '%') {
+type = LFT_PERCENT;
+++cur;
+}

Why do we want to start hiding these errors now? Is hiding them related
to this patch scope? Should we at least warn the admin that their page
may be buggy (but, as you know, I personally prefer a fatal exit on
configuration errors).


This is the bit of code which replaces the above "mb.Printf("%%%c", 
token);" removal.


If we use fatal() here anyone loading a template with future defined 
codes or typos will crash Squid every time that error is supposed to be 
displayed. The assumption that they meanst %% is not great, but we do 
have to choose between hiding the unknown token and displaying it 
un-expanded and since libformat tokens are variable in length we don't 
really have the hide option without potentially screwing something else 
up as well. This way at least it will show up during trivial testing by 
the admin.


Amos

Build failed in Jenkins: 3.HEAD-amd64-FreeBSD-9.0-clang #272

[noc]

See 


Changes:

[Alex Rousskov] Support forwarding intercepted but not bumped connections to 
cache_peers.

When talking to a cache_peer (i.e., sending a CONNECT request before tunneling
the transaction), tunnel code is using a clever hack: Squid does not parse
the CONNECT response from peer but blindly forwards it to the client. This
works great and simplifies code a lot, except when the client connection
was intercepted and, hence, the client did not send a CONNECT request and
is not expecting a CONNECT response.

In those situations, we now accumulate, parse, and strip the peer CONNECT
response (or close connection on errors).

The existing tunnel I/O code is too simple to accommodate that task -- it
cannot accumulate read data (its I/O buffers work in lockstep fashion, writing
everything it reads before reading again). Instead of rewriting the entire
tunnel code to use more complex buffers, I added a temporary accumulation
buffer for the CONNECT response. That buffer is not allocated unless it is
needed and does not grow beyond SQUID_TCP_SO_RCVBUF size, just like the
simple buffers.

--
[...truncated 17830 lines...]
mv -f .deps/Pages.Tpo .deps/Pages.Plo
/bin/sh ../../libtool  --tag=CXX   --mode=compile clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
  -I../../.. -I../../../include -I../../../lib  -I../../../src -I../../include  
-I/usr/local/include -I/usr/include  -I/usr/include -I../../../libltdl  
-I/usr/include  -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments  -D_REENTRANT -g -O2 -MT PageStack.lo -MD -MP -MF 
.deps/PageStack.Tpo -c -o PageStack.lo `test -f 'mem/PageStack.cc' || echo 
'../../../src/ipc/'`mem/PageStack.cc
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT PageStack.lo -MD -MP -MF 
.deps/PageStack.Tpo -c ../../../src/ipc/mem/PageStack.cc  -fPIC -DPIC -o 
.libs/PageStack.o
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT PageStack.lo -MD -MP -MF 
.deps/PageStack.Tpo -c ../../../src/ipc/mem/PageStack.cc  -fPIC -DPIC -o 
PageStack.o >/dev/null 2>&1
mv -f .deps/PageStack.Tpo .deps/PageStack.Plo
/bin/sh ../../libtool  --tag=CXX   --mode=compile clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
  -I../../.. -I../../../include -I../../../lib  -I../../../src -I../../include  
-I/usr/local/include -I/usr/include  -I/usr/include -I../../../libltdl  
-I/usr/include  -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments  -D_REENTRANT -g -O2 -MT Segment.lo -MD -MP -MF 
.deps/Segment.Tpo -c -o Segment.lo `test -f 'mem/Segment.cc' || echo 
'../../../src/ipc/'`mem/Segment.cc
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT Segment.lo -MD -MP -MF 
.deps/Segment.Tpo -c ../../../src/ipc/mem/Segment.cc  -fPIC -DPIC -o 
.libs/Segment.o
libtool: compile:  clang++ -DHAVE_CONFIG_H 
-DDEFAULT_STATEDIR=\"/usr
 -I../../.. -I../../../include -I../../../lib -I../../../src -I../../include 
-I/usr/local/include -I/usr/include -I/usr/include -I../../../libltdl 
-I/usr/include -I/usr/include -Werror -Wno-error=parentheses-equality 
-Qunused-arguments -D_REENTRANT -g -O2 -MT Segment.lo -MD -MP -MF 
.deps/Segment.Tpo -c ../../../src/ipc/mem/Segment.cc  -fPIC -DPIC -o Segment.o 
>/dev/null 2>&1
mv -f .deps/Segment.Tpo .deps/Segment.Plo
/bin/sh ../../libtool --tag=CXX--

Jenkins build is back to normal : 3.HEAD-amd64-FreeBSD-7.2 #1888

[noc]

See 

Jenkins build is back to normal : 3.HEAD-amd64-opensuse #523

[noc]

See