squid-2.6 build broken

[Adrian Chadd]

Hiya,

I've just updated to the latest squid-2.6 source and have found the
code isn't building. Any ideas?

[EMAIL PROTECTED]:~/work/squid/squid-2.6$ gcc -v
Using built-in specs.
Target: i486-linux-gnu
Configured with: ../src/configure -v 
--enable-languages=c,c++,java,f95,objc,ada,treelang --prefix=/usr 
--enable-shared --with-system-zlib --libexecdir=/usr/lib 
--without-included-gettext --enable-threads=posix --enable-nls 
--program-suffix=-4.0 --enable-__cxa_atexit --enable-clocale=gnu 
--enable-libstdcxx-debug --enable-java-awt=gtk-default --enable-gtk-cairo 
--with-java-home=/usr/lib/jvm/java-1.4.2-gcj-4.0-1.4.2.0/jre --enable-mpfr 
--disable-werror --with-tune=pentium4 --enable-checking=release i486-linux-gnu
Thread model: posix
gcc version 4.0.3 (Ubuntu 4.0.3-1ubuntu5)

This is on Ubuntu Dapper-beta.


[EMAIL PROTECTED]:~/work/squid/squid-2.6$ !124
./configure --prefix="/home/adrian/work/squid/run" --enable-storeio="ufs aufs 
coss null" --disable-poll --enable-epoll --with-large-files 
--enable-large-cache-files --quiet --with-maxfd=4096
Store modules built: ufs aufs coss null
aufs store used, pthreads support automatically enabled
coss store used, aio support automatically enabled
Removal policies built: lru
Forcing poll() to be disabled
Forcing epoll() to be enabled
Large cache file support enabled
Using POSIX_V6_ILP32_OFFBIG build environment
Hostname sanity checks enabled
Auth scheme modules built: basic
unlinkd enabled
Maximum filedescriptors set to 4096
Will use our own inet_ntoa().
[EMAIL PROTECTED]:~/work/squid/squid-2.6$ make
Making all in lib
make[1]: Entering directory `/home/adrian/work/squid/squid-2.6/lib'
if gcc -DHAVE_CONFIG_H -I. -I. -I../include -I../include -I../include-m32 
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -g -O2 -Wall -D_REENTRANT -MT 
Array.o -MD -MP -MF ".deps/Array.Tpo" -c -o Array.o Array.c; \
then mv -f ".deps/Array.Tpo" ".deps/Array.Po"; else rm -f 
".deps/Array.Tpo"; exit 1; fi
In file included from /usr/include/string.h:33,
 from Array.c:50:
/usr/lib/gcc/i486-linux-gnu/4.0.3/include/stddef.h:214: error: duplicate 
???unsigned???
/usr/lib/gcc/i486-linux-gnu/4.0.3/include/stddef.h:214: error: two or more data 
types in declaration specifiers
In file included from ../include/util.h:39,
 from Array.c:52:
/usr/include/time.h:172: error: two or more data types in declaration specifiers
In file included from /usr/include/sys/time.h:30,
 from ../include/util.h:44,
 from Array.c:52:
/usr/include/sys/select.h:85: error: two or more data types in declaration 
specifiers
make[1]: *** [Array.o] Error 1
make[1]: Leaving directory `/home/adrian/work/squid/squid-2.6/lib'
make: *** [all-recursive] Error 1
[EMAIL PROTECTED]:~/work/squid/squid-2.6$ 

Re: C++ question related to casting

[Robert Collins]

On Tue, 2006-05-23 at 21:40 +0200, Henrik Nordstrom wrote:
> tis 2006-05-23 klockan 15:01 -0400 skrev Nick Lewycky:
> 
> > If possible, it should store the HttpHeaderEntry objects themselves
> > instead of pointers to them. Done properly, there shouldn't be any need
> > for a destructor at all.
> 
> In this case there was a const barrier which needed to be penetrated,
> triggering all of this.. Probably a design error somewhere (missing
> write access method, or the code in question executing in the wrong
> context).
> 
> > if not possible, then HttpHeader should either have an explicit copy
> > constructor which does the right thing, or else be non-copyable by
> > adding unimplemented private copy constructor and operator= . See
> > http://www.boost.org/boost/noncopyable.hpp for the technique.
> 
> The latter I think here.. this kind of objects is best copied
> explicitly, and C++ is a bit too keen on making copy easy via the normal
> methods..

This is why, in our C++ guidelines we have 'always create copy
constructor and assignment operators - leave unimplemented if desired'.

:)

I'm not sure where they are now, but I remember discussing them way
back.

Rob
-- 
GPG key available at: .


signature.asc
Description: This is a digitally signed message part

Re: C++ question related to casting

[Henrik Nordstrom]

tis 2006-05-23 klockan 21:40 +0200 skrev Henrik Nordstrom:

> In this case there was a const barrier which needed to be penetrated,
> triggering all of this.. Probably a design error somewhere (missing
> write access method, or the code in question executing in the wrong
> context).

The code in question has now been cleaned up no longer requiring this
const barrier penetration, and the code audited to not have any other
places where HttpHeader gets abused in the same manner..

Unfortunately the assignment operator could not be blocked as it is
currently in use by the internal reset method.

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: external acl cache level

[Henrik Nordstrom]

tis 2006-05-23 klockan 11:08 -0300 skrev Gonzalo Arana:


> I would like to have a somehwat explicit cache level.  Alternatives to this 
> is:
> a) expand %| to some string: discarded.
> b) helper tells squid about format specification & cache levels upon startup.
> c) squid notifies helper at startup about format & cache levels.  If
> there is any inconsistency, helper can notify admin.
> 
> I prefer "b", but "c" sounds good as well.

So lets explore "b".

My initial thought was to have a standard command line argument making
the helper print it's request format definition if none is specified in
external_acl_type.

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: C++ question related to casting

[Henrik Nordstrom]

tis 2006-05-23 klockan 15:01 -0400 skrev Nick Lewycky:

> If possible, it should store the HttpHeaderEntry objects themselves
> instead of pointers to them. Done properly, there shouldn't be any need
> for a destructor at all.

In this case there was a const barrier which needed to be penetrated,
triggering all of this.. Probably a design error somewhere (missing
write access method, or the code in question executing in the wrong
context).

> if not possible, then HttpHeader should either have an explicit copy
> constructor which does the right thing, or else be non-copyable by
> adding unimplemented private copy constructor and operator= . See
> http://www.boost.org/boost/noncopyable.hpp for the technique.

The latter I think here.. this kind of objects is best copied
explicitly, and C++ is a bit too keen on making copy easy via the normal
methods..

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: C++ question related to casting

[Nick Lewycky]

Henrik Nordstrom wrote:
> tis 2006-05-23 klockan 13:22 -0400 skrev Nick Lewycky:
> 
> 
>>It's a temporary. pe->getReply()->head will get casted into the
>>HttpHeader, used to call putStr(...), then its life is over, at which
>>point the C++ implementation is free to destroy it at its leisure (some
>>impl's do it after the end of the statement, some wait until the end of
>>the function).
>>
>>Even if you'd said "HttpHeader HH = ...", it would still create a
>>temporary, copy-construct the new HttpHeader, and then destroy the
>>temporary.
> 
> 
> So why did the original get smashed when the temporary was destructed? A
> missing copy constructor?

Because the temporaries destructor called "clean()" which freed every
element in the entries vector. Realize that the copy was shallow, not
deep, so pointers stored in the class were duplicated, not copied properly.

If possible, it should store the HttpHeaderEntry objects themselves
instead of pointers to them. Done properly, there shouldn't be any need
for a destructor at all.

If not possible, then HttpHeader should either have an explicit copy
constructor which does the right thing, or else be non-copyable by
adding unimplemented private copy constructor and operator= . See
http://www.boost.org/boost/noncopyable.hpp for the technique.

Nick

Re: C++ question related to casting

[Henrik Nordstrom]

tis 2006-05-23 klockan 13:22 -0400 skrev Nick Lewycky:

> It's a temporary. pe->getReply()->head will get casted into the
> HttpHeader, used to call putStr(...), then its life is over, at which
> point the C++ implementation is free to destroy it at its leisure (some
> impl's do it after the end of the statement, some wait until the end of
> the function).
> 
> Even if you'd said "HttpHeader HH = ...", it would still create a
> temporary, copy-construct the new HttpHeader, and then destroy the
> temporary.

So why did the original get smashed when the temporary was destructed? A
missing copy constructor?

REgards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: C++ question related to casting

[Nick Lewycky]

Henrik Nordstrom wrote:
> Hi Robert,
> 
> can you (or someone else who groks C++ casts) please explain why this
> happened?
> 
> http://www.squid-cache.org/Versions/v3/3.0/changesets/10249.patch
> 
> 
> store.cc:712
> 
>   ((HttpHeader) pe->getReply()->header).putStr(HDR_VARY, vary.buf());

It's a temporary. pe->getReply()->head will get casted into the
HttpHeader, used to call putStr(...), then its life is over, at which
point the C++ implementation is free to destroy it at its leisure (some
impl's do it after the end of the statement, some wait until the end of
the function).

Even if you'd said "HttpHeader HH = ...", it would still create a
temporary, copy-construct the new HttpHeader, and then destroy the
temporary.

Nick Lewycky

C++ question related to casting

[Henrik Nordstrom]

Hi Robert,

can you (or someone else who groks C++ casts) please explain why this
happened?

http://www.squid-cache.org/Versions/v3/3.0/changesets/10249.patch


store.cc:712

((HttpHeader) pe->getReply()->header).putStr(HDR_VARY, vary.buf());

called the ~HttpHeader destructor, which made a mess of everything (it's not 
supposed to be destroyed yet...)


pe is a StoreEntry

getReply returns a const HttpReply.


rep is assigned a few lines earlier

HttpReply *rep = (HttpReply *) pe->getReply();  // bypass const

and using this saved our day..  but I don't quite understand why the
original line made such a mess of things. Feels a bit magic..


Valgrind trace which took me there:

(18:01:38) hno: valgrind traps bad memory references.. just had to disable 
memory pools..
(18:02:23) hno: ==25140== Invalid read of size 2
(18:02:23) hno: ==25140== at 0x41236C: String::size() const (String.cci:46)
(18:02:23) hno: ==25140== by 0x488E40: HttpHeaderEntry::packInto(Packer*) const 
(HttpHeader.cc:1422)
(18:02:23) hno: ==25140== by 0x488F24: HttpHeader::packInto(Packer*) const 
(HttpHeader.cc:615)
(18:02:23) hno: ==25140== by 0x4901B6: HttpReply::packHeadersInto(Packer*) 
const (HttpReply.cc:128)
(18:02:23) hno: ==25140== by 0x4CAA92: storeSetPublicKey (store.cc:734)
(18:02:23) hno: ==25140== by 0x4CACF5: StoreEntry::makePublic() (store.cc:184)
(18:02:23) hno: ==25140== by 0x4813DC: HttpStateData::haveParsedReplyHeaders() 
(http.cc:838)
(18:02:23) hno: ==25140== by 0x4817E1: HttpStateData::processReplyHeader() 
(http.cc:778)
(18:02:23) hno: ==25140== by 0x481C97: HttpStateData::readReply(unsigned long, 
comm_err_t, int) (http.cc:1089)
(18:02:23) hno: ==25140== by 0x481DD6: HttpStateData::ReadReplyWrapper(int, 
char*, unsigned long, comm_err_t, int, void*) (http.cc:970)
(18:02:23) hno: ==25140== by 0x4E3233: CommReadCallbackData::callCallback() 
(comm.cc:399)
(18:02:23) hno: ==25140== by 0x4E7629: CommCallbackData::callACallback() 
(comm.cc:438)
(18:02:23) hno: ==25140== Address 0x5492C72 is 10 bytes inside a block of size 
40 free'd
(18:02:23) hno: ==25140== at 0x4905208: free (vg_replace_malloc.c:235)
(18:02:23) hno: ==25140== by 0x524447: xfree (util.c:481)
(18:02:23) hno: ==25140== by 0x51A792: MemMalloc::deallocate(void*) 
(MemPool.cc:522)
(18:02:23) hno: ==25140== by 0x51A93C: MemImplementingAllocator::free(void*) 
(MemPool.cc:538)
(18:02:23) hno: ==25140== by 0x51C997: MemAllocatorProxy::free(void*) 
(MemPool.cc:847)
(18:02:23) hno: ==25140== by 0x48CF6C: HttpHeaderEntry::operator delete(void*) 
(HttpHeader.h:184)
(18:02:23) hno: ==25140== by 0x48C1AF: HttpHeader::clean() (HttpHeader.cc:401)
(18:02:23) hno: ==25140== by 0x48C203: HttpHeader::~HttpHeader() 
(HttpHeader.cc:362)
(18:02:23) hno: ==25140== by 0x4CA917: storeSetPublicKey (store.cc:712)


Thanks again Andre for making it easy to disable mempool chunking,
without that capability this would have been nearly impossible to see.

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: Squid-2.6 snapshots request

[Henrik Nordstrom]

ons 2006-05-24 klockan 00:43 +1200 skrev Reuben Farrelly:
> Hi Henrik,
> 
> Would you be able to set up a cron job to create 2.6 snapshots, the same as 
> we 
> do for 3.0?

As soon as the merge frenzy is over there will be a PRE release,
snapshots, version page, releae notes etc.. all the things you are used
to for a Squid release.

> http://www.squid-cache.org/Versions/v2/2.5/
> 
> I find it useful to keep one or two snapshots aside in case I need to roll 
> back 
> if some breakage occurs in CVS.

rollback is trivial with CVS..

  cvs update -D 2006-05-22

(you can go down to minutes if you like..)

To go back to HEAD

  cvs update -r HEAD 

> I'm using 2.6 from CVS now and it works pretty good, with the exception of 
> WCCPv2 (which does not work at all here).

Please file a bug report so it is not overlooked.

> It's certainly very good to see so many more people contributing than before 
> the 
> 2.6 merge commenced.  I hope the momentum carries forward to 3.0 after the 
> 2.6 
> cycle is done.

Nearly everything except pinning and COSS is in Suqid-3 already..

there is some also bugfixes in the 2.6 code not found in squid-3.

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: external acl cache level

[Gonzalo Arana]

Just to summ up:

Letting the helper do random combining/reordering leads into
highly-ineffitient lookup algorithm, and apparently it is not needed.

Cache level could structured in a 'path'-alike, or disjoint.  Disjoint
solves reordering issue in an efficient manner.

Having each token a member of a cache level is discarded as an option.

I would like to have a somehwat explicit cache level.  Alternatives to this is:
a) expand %| to some string: discarded.
b) helper tells squid about format specification & cache levels upon startup.
c) squid notifies helper at startup about format & cache levels.  If
there is any inconsistency, helper can notify admin.

I prefer "b", but "c" sounds good as well.

--
Gonzalo A. Arana

Connection pinning (patch)

[Steven Wilton]

Hi,

Adrian asked me to check the connection pinning code in HEAD (as we're 
actually using it on our network), and I can see a couple of problems. 
I've attached a diff that should fix them.


The first part of this patch will make sure we only mark a requset with the 
pinned and auth flags if there is a server-side persistent connection 
waiting.  This will stop extra server-side fd's from being marked as pinned. 
The second part of the patch makes sure the correct code is followed after a 
pconnPop() for pinned and tproxy connections.


Steven 


pinning-2.6.patch
Description: Binary data

Re: external acl cache level

[Henrik Nordstrom]

mån 2006-05-22 klockan 23:17 -0300 skrev Gonzalo Arana:

> Ah, I see now.  As long as the helper & squid follow "lower level
> number, higher priority" policy, there is no need for cache
> invalidation.

Correct.

> > To be able to make sane lookup structures it is very beneficial if the
> > data can be structured in a path like structure. This worked out quite
> > okay except that there is acl types where the acl arguments (the data in
> > the acl statement) is more important than some request details
> > (external_acl_type format tags)...
> 
> I may be wrong, but reordering is needed in those cases, which is why
> I proposed 'combining' key components: letting the helper specify
> which request-tokens may be used for caching this response.

Problem with supporting dynamic reordering like this (different from the
fixed cache levels) is that you then need to perform a 2^N lookup
instead of linear N. For 1-2 levels this is no difference, but
complexity quickly grows with the number of levels.

> Sure! Here is an example:
> external_acl_type useless %{Host} %| /some/helper some-argument
> acl yet_another_useless external useless %{Cookie} %| %{MYADDR}
> 
> We could just demand that /some/helper should be aware of request
> levels (this is something you pointed out below).  Sooner or later
> this will lead to confusions.

I honestly do not see a problem with that. Anyway we demand that the
helper knows which arguments it will get, which implies that the we
demand that the admin knows what external_acl_type definition should be
used.

But I am not a strong supporter of allowing format tags within the acl
data. These belong in the external_acl_type. Gets too messy otherwise
with high risk of misconfiguration I think, and I don't see many
practical situations where it would help.

The above should be expressed as

external_acl_type useless %{Host} %| %{Cookie} %| %{MYADDR} %| some/helper 
some-argument
acl yet_another_useless external useless YetMoreUselessInfo

> Options:
> 1) To expand '%|' to some string that we know it won't be present in
> any other tags.  I fear no matter which string we choose for '%|'
> expansion; that string could be present in (for instance) Cookie
> request header.

I think it is better left outside of the helper protocol. If there
should be a change then perhaps adding a startup message where Squid
announces to the helper the protocol used, allowing the helper to verify
the configuration and alert the admin on errors..

> 2) As you proposed:
> > Another approach would be to mark the arguments per their key detail level.
> Unless I misunderstand this, you are proposing that each request could
> look something like this (I know that there are cleaner ways to do it,
> this is just an example):
> 1=localhost 1=blah1 2=user_xxx 3=1.1.1.1
> where each integer represent the key level.
> With this approach, key-component level is assigned by squid
> configuration, and is not per-request (which perhaps is what is
> wanted).

I was more thinking on allowing the cache levels to be disjoint. I.e.

level1 = %LOGIN
level2 = %HOST
level3 = %LOGIN %HOST
level4 = %LOGIN acldata

instead of just a linear path like today.

level1 = %LOGIN
level2 = %LOGIN %HOST
level3 = %LOGIN %HOST acldata

> 3) We could let external helper to decide key-component level by using
> something like XMLRPC or we could come up with our own protocol based
> in, say, HTTP.

Problem is again how to maintain the lookup cache in an efficient manner
if the levels and their priorities is not static.

> This encoding/protocol/structure (whatever this should be called)
> should add support for something like HTTP's Vary: in the response,
> the helper should indicate which components of the request were taken
> into consideration for building the reply.

After years of study I have yet to come up with any lookup structure
supporting HTTP's Vary in an efficient manner. It's a very complex
thing.

It's manageable if one assumes there is a single Vary specification per
URL (which translates to external_acl_type in this discussion I would
say), but the HTTP specifications do not really place this limitation so
servers can respond with different Vary specifications in different
responses of the same URL making the problem explode.. (and making heads
explode when trying to explain the cache effects of doing so..)

> Let me see If I follow correctly: with %DATA you can switch the order
> of the arguments to external_acl, right?  So you can make acl
> arguments have higher priorities than external_acl formats.

Yes.

> > Problem: %DATA have a slight problem with whitespace characters if the
> > helper is to handle arguments with whitespace AND multiple arguments in
> > the same acl type.. as currently written they both looks the same in the
> > %DATA expansion.. (a space character, appropriately encoded per the
> > helper protocol).
> 
> we seem to fall into "some higher level structure is needed" again.
> Mainly because the external helper 

Re: Squid-2.6 update

[Henrik Nordstrom]

tis 2006-05-23 klockan 04:03 -0400 skrev Tsantilas Christos:

> icap-2.6 branch already exists in sourceforge cvs repository.

Excellent!

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: Squid-2.6 update

[Tsantilas Christos]

Hi all,
> tor 2006-05-18 klockan 01:59 +0200 skrev [EMAIL PROTECTED]:
>
>> Just a question, (i've been quite busy those weeks to follow this ml),
>> is the
>> ICAP support already merged ? I'd like to help Christos if not
>

icap-2.6 branch already exists in sourceforge cvs repository.
Looks that mostly works with other patches/features disabled.
I made the last merge with HEAD at sunday
We must check it more to find what parts work well,
and what parts do not work.

> ICAP is currently not a candidate for merging into 2.6 as from what I
> know the icap patch is not considered production quality.

At this time squid-icap has some stability problems and does not
interoperate always  well with other squid features.

Regards,
Christos