Re: cvs commit: squid3/src HttpHdrContRange.cc Makefile.in MemObject.cc access_log.cc cache_cf.cc client_side.cc client_side_reply.cc stmem.cc
Hi , Henrik Nordstrom wrote: hno 2007/08/13 12:25:14 MDT Modified files: src HttpHdrContRange.cc Makefile.in MemObject.cc access_log.cc cache_cf.cc client_side.cc client_side_reply.cc stmem.cc Log: Fix a few more broken or missing casts Some of them was not just broken or missing casts but real bugs.. I tried to be careful but Sorry. I hope that I did not make other bugs ... Regards, Christos
Re: Squid 3 build error using Visual Studio
Hi Alex, At 18.43 13/08/2007, Alex Rousskov wrote: This is with the code recently synced with HEAD, right? Please try the attached untested patch. It follows the suggestions at the page found by Amos: http://www.codecomments.com/archive292-2005-2-396222.html Yes, it works. Thanks. Thanks for the link, but, I have really understand nothing :-( Really I hate C++ :-( Is it a good idea to hate something you do not understand? :-) Alex, let me to explain my situation. Principally I'm an IT consultant working (busy at 110% of my time) on Systems Administration and Systems Deployment, while I became a Squid developer mainly for my personal hobby. I have learned at university (more than 15 years ago ...) Assembler, Fortran, C and Pascal, but never C++. or any similar language. I'm not a wizard of C language programming or a Squid guru like Henrik, but I was always able to understand what a piece of code of Squid 2 does. Now I feel really frustrated when I can't understand why a single line of code of Squid 3 doesn't build or why your patch works :-( I have also tried to learn C++ by myself, but with little result because I don't have enough time for the job. For a while I'm asking to me if is time to look for a new maintainer of the Windows port of Squid 3, and today I'm thinking that probably this time is gone ... :-( Regards Guido - Guido Serassio Acme Consulting S.r.l. - Microsoft Certified Partner Via Lucia Savarino, 1 10098 - Rivoli (TO) - ITALY Tel. : +39.011.9530135 Fax. : +39.011.9781115 Email: [EMAIL PROTECTED] WWW: http://www.acmeconsulting.it/
StoreIOBuffer field types
As part of the checks to find possible causes of my servers falling over
I went to see if coverity had any more issues located.
One of them was unsafe use of a variable typed 'size_t' setting up
StoreIOBUffer local variable
The constructor using it is:
StoreIOBuffer(size_t aLength, int64_t anOffset, char *someData) :
length (aLength), offset (anOffset), data (someData)
{
flags.error = 0;
}
where length/aLength are the size_t in question.
I had thought it the length of the buffer and offset in the buffer. But
seeing the offset is now int64, is it the lengths of the file being
buffered? In which case it should be a 64-bit too I think.
Or was I right about being buffer length? in which case it should be an
unsigned something? which prevents this warning showing.
Amos
Re: cvs commit: squid3/src ftp.cc
Alex Rousskov wrote: On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote: Now fixed. If you have time, it may be better to replace a virtual FtpStateData::haveControlChannel(char*) into a static FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check the ftpState pointer itself. This will avoid more code repetition and may even work faster. Whats with the uppercasing? the method is not a type. Is this a new convention not mentioned in the wiki/devel-faq? The FTP code is all a it messy still with its partial conversion and overlaps with other protocols code. When the whole area is cleaned up several of those functions should be inline. I was wondering about an *this check inside (does away with the extraneous parameter), but I'm not certain enough to code it for 3.0. I would also suggest to use a different method name since the code assumes it is only called to detect attempts to call a method when a control channel is open. Also, doneWithServer() actually checks for the presence of both data and control channels. Ah, quite right. doneWithServer() is only one of the cases that method should handle. I'll be adding the other soon. Amos
Re: Squid 3 build error using Visual Studio
Hi Guido , Guido Serassio wrote: I have learned at university (more than 15 years ago ...) Assembler, Fortran, C and Pascal, but never C++. or any similar language. Sometimes I am feeling to old to start thinks too, but Guido if you are thinking like that now, what are we going to do after 15 years? New technologies come again and again ... Now I feel really frustrated when I can't understand why a single line of code of Squid 3 doesn't build or why your patch works :-( The major problem you may have is that c++ compilers some times return error messages which looks completely unrelated with the real problem. In your case it returned an error about std::basic_ostream where the real problem is that it can not found a destructor without arguments for std::ostream part of StoreEntryStream objects... (I had similar problem today morning trying to understand what the hell means the undefined reference to vtable for .) But I think as someones learns the compilers and tools which his is using these problems disappeared Regards, Christos
Re: StoreIOBuffer field types
Hi Amos,
Amos Jeffries wrote:
One of them was unsafe use of a variable typed 'size_t' setting up
StoreIOBUffer local variable
The constructor using it is:
StoreIOBuffer(size_t aLength, int64_t anOffset, char *someData) :
length (aLength), offset (anOffset), data (someData)
{
flags.error = 0;
}
where length/aLength are the size_t in question.
I had thought it the length of the buffer and offset in the buffer. But
seeing the offset is now int64, is it the lengths of the file being
buffered? In which case it should be a 64-bit too I think.
The StoreIOBuffer::length is buffer size and StoreIOBuffer::offset is
the offset inside the objects which must be 64 bit integer to allow
large objects passing through squid3.
I think it must be 64bit even if storing large objects does not
supported (it is not just an offset of a stored object) and this is why
the int64_t type used instead of off_t type.
Regards,
Christos
Re: Squid 3 build error using Visual Studio
On Tue, 2007-08-14 at 11:12 +0200, Guido Serassio wrote: At 18.43 13/08/2007, Alex Rousskov wrote: Is it a good idea to hate something you do not understand? :-) Alex, let me to explain my situation. Principally I'm an IT consultant working (busy at 110% of my time) on Systems Administration and Systems Deployment, while I became a Squid developer mainly for my personal hobby. I have learned at university (more than 15 years ago ...) Assembler, Fortran, C and Pascal, but never C++. or any similar language. I'm not a wizard of C language programming or a Squid guru like Henrik, but I was always able to understand what a piece of code of Squid 2 does. Now I feel really frustrated when I can't understand why a single line of code of Squid 3 doesn't build or why your patch works :-( I have also tried to learn C++ by myself, but with little result because I don't have enough time for the job. For a while I'm asking to me if is time to look for a new maintainer of the Windows port of Squid 3, and today I'm thinking that probably this time is gone ... :-( I hope you can continue to maintain the Windows port. You can always offload solving C++ puzzles to others... The specific bug you hit is with the low-level streaming code that has always been very difficult to write and port correctly because C++ libraries implemented low-level streaming interfaces differently. In your case, GCC stdlibc++ has a default std::ostream constructor while your Windows library does not, causing the compiler error. The rest is just hacks to make the existing parameterized std::ostream constructor work. And if you continue to hate C++, just wait for Squid4 in Python :-). There will be no compile-time errors then, even if half of the code is missing! Alex.
Re: StoreIOBuffer field types
On tis, 2007-08-14 at 22:40 +1200, Amos Jeffries wrote: Or was I right about being buffer length? in which case it should be an unsigned something? which prevents this warning showing. My understanding is that the length is the in-memory buffer length, and offset is the offset within the object. But not 100% sure. Regards Henrik signature.asc Description: This is a digitally signed message part
Re: cvs commit: squid3/src ftp.cc
On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote: Alex Rousskov wrote: On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote: Now fixed. If you have time, it may be better to replace a virtual FtpStateData::haveControlChannel(char*) into a static FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check the ftpState pointer itself. This will avoid more code repetition and may even work faster. Whats with the uppercasing? the method is not a type. Is this a new convention not mentioned in the wiki/devel-faq? Hmm.. it should be in the style guide somewhere. object local methods and variables start with a lowercase, global variables and static class members with an upper case. The FTP code is all a it messy still with its partial conversion and overlaps with other protocols code. Can you be specific on what you think need further conversion or overlaps? Ah, quite right. doneWithServer() is only one of the cases that method should handle. I'll be adding the other soon. And quite likely the whole function is not needed any more after the state fixes, but it's a reasonable safeguard so I don't object it's addition. Regards Henrik signature.asc Description: This is a digitally signed message part
Re: IPv6 support and Windows
On tis, 2007-08-14 at 13:41 +0200, Guido Serassio wrote: I'm not sure if the licensing of the implementation coming from the PostgreSQL allow its usage in Squid. Looks fine. PostgreSQL is licensed under the modified/modern three-clause BSD license, which is fully GPL compatible. http://www.postgresql.org/about/licence Regards Henrik signature.asc Description: This is a digitally signed message part
Re: Squid 3 build error using Visual Studio
On tis, 2007-08-14 at 14:10 +0300, Tsantilas Christos wrote: The major problem you may have is that c++ compilers some times return error messages which looks completely unrelated with the real problem. Or quite related but where it's impossible to grok what is related or how... http://www.henriknordstrom.net/code/loreley_warning.html (I had similar problem today morning trying to understand what the hell means the undefined reference to vtable for .) My experience: Missing object file implementing the class, only having header references. But if I hadn't known already that the vtable is what the object internal table of virtual methods is called I would not have understood that error much at all.. But I think as someones learns the compilers and tools which his is using these problems disappeared And then the tools gets upgraded and you have to learn new errors.. The problem with C++ compared to C is complexity. C++ is a fairly complex language with many levels of abstraction and a lot of hidden magics which needs to be understood apart from the syntax itself. This makes a very steep learningcurve when trying to understand existing code. Tools like doxygen helps a lot to decipher unknown code. Without them reading and understanding complex C++ code is a nightmare. But this is also where the powers of C++ lies. The abstraction levels do allows for clean code to be done easier than in for example C. The Squid-3 code is currently a bit messy with 4-5 differen generations of coding style. But gradually things do get cleaned up. And I am also looking forward to the docs project getting merged which is a significant step along that path imho. Regards Henrik signature.asc Description: This is a digitally signed message part
Re: cvs commit: squid3/src ftp.cc
On Tue, 2007-08-14 at 17:58 +0200, Henrik Nordstrom wrote: On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote: Alex Rousskov wrote: On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote: Now fixed. If you have time, it may be better to replace a virtual FtpStateData::haveControlChannel(char*) into a static FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check the ftpState pointer itself. This will avoid more code repetition and may even work faster. Whats with the uppercasing? the method is not a type. Is this a new convention not mentioned in the wiki/devel-faq? Hmm.. it should be in the style guide somewhere. http://www.squid-cache.org/Devel/squid-3-style.txt object local methods and variables start with a lowercase, global variables and static class members with an upper case. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: cvs commit: squid3/src ftp.cc
On Tue, 2007-08-14 at 17:58 +0200, Henrik Nordstrom wrote: On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote: Alex Rousskov wrote: On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote: Now fixed. If you have time, it may be better to replace a virtual FtpStateData::haveControlChannel(char*) into a static FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check the ftpState pointer itself. This will avoid more code repetition and may even work faster. Whats with the uppercasing? the method is not a type. Is this a new convention not mentioned in the wiki/devel-faq? Hmm.. it should be in the style guide somewhere. http://www.squid-cache.org/Devel/squid-3-style.txt What I based the following on. Alex posted an upper-cased internal-but-public method (NOT data member). object local methods and variables start with a lowercase, global variables and static class members with an upper case.
Re: cvs commit: squid3/src BodyPipe.cc BodyPipe.h
On Tue, 2007-08-14 at 09:54 +1200, Amos Jeffries wrote: rousskov2007/08/13 10:48:20 MDT snip The new code also keeps track of the number of outstanding events and skips that number if the consumer leaves. TODO: when AscyncCall support is improved, should we just schedule calls directly to consumer? It could be a much cleaner solution than counting pending calls and skipping them when needed. You mean like client-streams was meant to be (I think, after only a few days reading the code and a bias from work on other real-time applications). Where only the tail gets event calls and read events pull as much buffer data through the chain as possible? No, nothing that complex! Chains are evil :-). BodyPipe is not a chain. It is a dumb connection/transport between two independent but cooperating jobs: the body consumer and the body producer. Currently, BodyPipe asynchronously calls itself so that it can synchronously call the consumer or the producer with the right parameters when new data arrives, etc. This creates a problem if the consumer is not there when (or changes after) the asynchronous call is scheduled: The new consumer may get only some of the scheduled calls, and things will not work well. When asynchronous calls are no longer hacked on top of a single-parameter Event, the above indirection will not longer be needed. BodyPipe will be able to asynchronously call the consumer or the producer with the right parameters. At that time, the committed fix, and 80% of current call wrapping functions will no longer be needed. Alex.
Re: IPv6 support and Windows
Hi Amos, Not good news here ... :-( We have a problem: You have used getaddrinfo() and freeaddrinfo() functions, but on Windows they are available only starting from Windows XP. This means that your code, even if compiled without IPv6 support, cannot run on Windows 2000. I'm also not sure if getaddrinfo() and freeaddrinfo() functions are available on all systems, someone know something about ? We have already tested Linux 2.6+ and FreeBSD 4.5+ (4.3 _says_ it has them) Not sure about Solaris, OS2, or Darwin yet. I have found two interesting substitute here: AYJ: pulled these out of order due to my response sequence http://doxygen.postgresql.org/getaddrinfo_8c-source.html I'm not sure if the licensing of the implementation coming from the PostgreSQL allow its usage in Squid. PostGres looks good until I audit the code. These are both killer bugs for us: Bugs: - only one addrinfo is set even though hintp is NULL or ai_socktype is 0 - AI_CANONNAME is not supported. http://www.koders.com/c/fid11C8C256BE9D1C263E0725F657383F62F24759F5.aspx?s=md5 The very first line says it's a (partial) implementation which makes me think there are edge cases missing, etc. Though a quick check of the code I couldn't see any obvious problems or notes about killer bugs. I'm sure there are more re-writes around, and sure enough Google codesearch find me a clean fetchmail implementation of JUST getaddrinfo(), freeaddrinfo() and gai_strerror(). ftp://ftp.sunfreeware.com/pub/freeware/SOURCES/fetchmail-6.3.6.tar.gzcs_f=fetchmail-6.3.6/libesmtp/getaddrinfo.c#a0 That is probably better for us being more complete in the IPv4-area. The crunch now will be whether getnameinfo() is also that unsupported in NT/2k ? AND its already under GNU license to FSF. Amos
Re: cvs commit: squid3/src BodyPipe.cc BodyPipe.h
On Tue, 2007-08-14 at 09:54 +1200, Amos Jeffries wrote: rousskov2007/08/13 10:48:20 MDT snip The new code also keeps track of the number of outstanding events and skips that number if the consumer leaves. TODO: when AscyncCall support is improved, should we just schedule calls directly to consumer? It could be a much cleaner solution than counting pending calls and skipping them when needed. You mean like client-streams was meant to be (I think, after only a few days reading the code and a bias from work on other real-time applications). Where only the tail gets event calls and read events pull as much buffer data through the chain as possible? No, nothing that complex! Chains are evil :-). BodyPipe is not a chain. It is a dumb connection/transport between two independent but cooperating jobs: the body consumer and the body producer. Currently, BodyPipe asynchronously calls itself so that it can synchronously call the consumer or the producer with the right parameters when new data arrives, etc. Aha! gotcha. I hope this is all commented in the code. I haven't got the docs branch around to ICAP yet. This creates a problem if the consumer is not there when (or changes after) the asynchronous call is scheduled: The new consumer may get only some of the scheduled calls, and things will not work well. When asynchronous calls are no longer hacked on top of a single-parameter Event, the above indirection will not longer be needed. BodyPipe will be able to asynchronously call the consumer or the producer with the right parameters. At that time, the committed fix, and 80% of current call wrapping functions will no longer be needed. Alex. Thanks Amos
Re: cvs commit: squid3/src ftp.cc
On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote: Alex Rousskov wrote: On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote: Now fixed. If you have time, it may be better to replace a virtual FtpStateData::haveControlChannel(char*) into a static FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check the ftpState pointer itself. This will avoid more code repetition and may even work faster. Whats with the uppercasing? the method is not a type. Is this a new convention not mentioned in the wiki/devel-faq? Hmm.. it should be in the style guide somewhere. object local methods and variables start with a lowercase, global variables and static class members with an upper case. The FTP code is all a it messy still with its partial conversion and overlaps with other protocols code. Can you be specific on what you think need further conversion or overlaps? Ah, sorry, I said that without taking a good overview look at it (yay doxygen). At the line-level it seems to have a lot of global stuff. I must have gotten FTP confused with the HttpRequest usages. The main gist of my thinking already exists in the form of ServerStateData for FTP at least. Alex made some comments earlier about use of temporary buffers in several of the protocol automata. And here is a short list of currently global functions which I think should be provided either by the main body of squid (non-FTP specific) or part of the classes private methods. is_month() escapeAC() ftpOpenListenSocket() ftpFail() ftpUrlWith2f() - should be part of URL class when its ready ftpTrySlashHack() (maybe others, I haven't looked closely at each one yet) maybe as ftpListParts open class: ftpListPartsFree() ftpListPartsParse() Ah, quite right. doneWithServer() is only one of the cases that method should handle. I'll be adding the other soon. And quite likely the whole function is not needed any more after the state fixes, but it's a reasonable safeguard so I don't object it's addition. truely. Amos
Re: Squid 3 build error using Visual Studio
On tis, 2007-08-14 at 14:10 +0300, Tsantilas Christos wrote:
The major problem you may have is that c++ compilers some times return
error messages which looks completely unrelated with the real problem.
Or quite related but where it's impossible to grok what is related or
how...
http://www.henriknordstrom.net/code/loreley_warning.html
(I had similar problem today morning trying to understand what the hell
means the undefined reference to vtable for .)
My experience: Missing object file implementing the class, only having
header references. But if I hadn't known already that the vtable is what
the object internal table of virtual methods is called I would not have
understood that error much at all..
But I think as someones learns the compilers and tools which his is
using these problems disappeared
And then the tools gets upgraded and you have to learn new errors..
The problem with C++ compared to C is complexity. C++ is a fairly
complex language with many levels of abstraction and a lot of hidden
magics which needs to be understood apart from the syntax itself. This
makes a very steep learningcurve when trying to understand existing
code.
Tools like doxygen helps a lot to decipher unknown code. Without them
reading and understanding complex C++ code is a nightmare.
But this is also where the powers of C++ lies. The abstraction levels do
allows for clean code to be done easier than in for example C.
The Squid-3 code is currently a bit messy with 4-5 differen generations
of coding style. But gradually things do get cleaned up. And I am also
looking forward to the docs project getting merged which is a
significant step along that path imho.
:-) Thanks. Even if it was your idea in the first place.
Guido (or anyone);
if you want to use the docs project to understand any code its public at
http://squid.treenet.co.nz/Doc/Code/
Two notes though for now:
- there is presently some breakage caused by those semi-colon-free macros:
void orphan() {}
MACRO(x)
void fooBarInvisible() { orphan(); }
- MACRO will screw up so docs will not include fooBarInvisible() and
details for orphan() will say its never referenced.
So for the present, unless a function has a docs comment or is linked from
the module page directly it needs checking for use manually.
Overall though its good now for a quick check of inheritance details and
what belongs where.
Amos
Re: cvs commit: squid3/src ftp.cc
On ons, 2007-08-15 at 09:28 +1200, Amos Jeffries wrote: If you have time, it may be better to replace a virtual FtpStateData::haveControlChannel(char*) into a static FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check the ftpState pointer itself. This will avoid more code repetition and may even work faster. Whats with the uppercasing? the method is not a type. Is this a new convention not mentioned in the wiki/devel-faq? Hmm.. it should be in the style guide somewhere. http://www.squid-cache.org/Devel/squid-3-style.txt What I based the following on. Alex posted an upper-cased internal-but-public method (NOT data member). It's a static class function, not an object method, which brings it into the Global naming recommendation. static FtpStateData::HaveControlChannel(FtpStateData*, char*) Regards Henrik signature.asc Description: This is a digitally signed message part
Re: IPv6 support and Windows
Hi Amos, Not good news here ... :-( We have a problem: You have used getaddrinfo() and freeaddrinfo() functions, but on Windows they are available only starting from Windows XP. This means that your code, even if compiled without IPv6 support, cannot run on Windows 2000. I'm also not sure if getaddrinfo() and freeaddrinfo() functions are available on all systems, someone know something about ? We have already tested Linux 2.6+ and FreeBSD 4.5+ (4.3 _says_ it has them) Not sure about Solaris, OS2, or Darwin yet. I have just imported and properly wrapped the libesmtp getaddrinfo and supporting implementation from fetchmail 6.3.8. I have done some simple configure and compile tests here. Its time to pass it back to you Guido for the win32 test. I'm kind of expecting another set of errors off these. It's just a matter of which ones. Amos
squid-3 ?
How long before the next pre or release candidate of squid-3 ? Adrian
