Re: a simple formatter
On Fri, 2008-02-08 at 23:26 +0200, Tsantilas Christos wrote: Alex Rousskov wrote: Changes in the current code are only bad for outstanding patches and forgotten branches, right? If everybody applies the same formatting to all their branches and HEAD, we should not have many conflicts, right? I don't know, possibly it is OK. Or just format the HEAD, merge HEAD to branches (this will take some hours but...) and then apply formating to the branch. After the first time all will be OK. with CVS you will be much better off formatting all branches simultaneously, then merging. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: X-Vary-Options patch
On Fri, 2008-02-08 at 16:26 +1100, Tim Starling wrote: The added features of the patch are conditional, and are enabled by the configure option --enable-vary-options. Unless there is non-trivial process required for regular vary headers with this enabled, I don't think it needs to be optional. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: two xasserts in squid3
On Mon, 2008-02-11 at 12:18 +0900, Adrian Chadd wrote: On Mon, Feb 11, 2008, Amos Jeffries wrote: That only because none has shown me a better way to do function pointers than the way squid currently does them. I'm pretty sure the async calls stuff has an implementation of this. There's other ways - look at asio.sourceforge.net's implementation which uses some Boost methods for creating 0, 1 and 2 argument callbacks of varying types. Do Boost methods for creating callbacks differ substantially from Squid methods? I was under impression they are about the same except Squid ones are less polished and simpler because they cover fewer special cases (that Squid is not using yet). Thank you, Alex.
Re: two xasserts in squid3
On Mon, 2008-02-11 at 10:15 +1300, Amos Jeffries wrote: I think a better approach to this would be: 1) do we actually need it anyway? Yes, we need a common assert-like macro. 2) where is it supposed to be defined? IMO: include/xassert.h if xassert is used outside of src/ src/xassert.h otherwise, until we get something like src/base/ or src/misc for general-purpose commonly used things that do not fit anywhere else. I'm willing to commit time to that if we can agree on the 'old' files that need removing and the scope of the remining ones. I have seen a few things that made me think squid.h and defines.h may need to (merge?) stick around under a better definition of content. I would welcome such cleanup, especially after we switch to a VCS that does file renaming. IMO, there needs to be one header file like squid.h that is guaranteed to be the first one included by all other source files in src/. That file should only contain things needed by 99.9% of the code base. This is usually limited to hacks that change some global system or compiler effects. The rest should have their own purpose-specific header files. If the source code needs something used by others, it needs to include something.h. There is also a question of whether some lib/ stuff needs to be moved to src/, but that is a separate debate and we need a better VCS for that as well. HTH, Alex.
Re: two xasserts in squid3
On Tue, 2008-02-12 at 11:10 +1300, Amos Jeffries wrote: On Mon, 2008-02-11 at 09:33 +1100, Robert Collins wrote: One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. Yes, of course. That would be the focus of the cleanup that Amos is volunteering to do :-) Hows this for an action plan Robert? * Obsolete typedefs.h (underway) - remove all unneeded typedefs - move all needed typedefs to their appropriate headers - fix compile errors * Add automatic testing for header dependency - script to perform universal include unit-test for .h files - link to automatic unit-testing in each directory - fix the compile errors! * Obsolete protos.h - move all protos to their appropriate header files - add includes for headers where needed. * Obsolete structs.h - move all structs to their appropriate header files - move modular configuration in to *Config.h files (discussion on exactly what the modules are) I like the automatic include unit test. I found while slowly working through this that circular structure dependencies made it quite tricky; I think you will find you need * Break dependencies to make it really clean, but that can be longer term if needed. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: two xasserts in squid3
On Tue, 2008-02-12 at 10:37 +1300, Amos Jeffries wrote:
When we get a better VCS, we should discuss moving include/ and lib/
stuff into src/ with the exception of 3rd party code. This would avoid
problems created by that artificial boundary.
What I have been mulling over after seeing code heirarchy like this:
/include + /lib == C library functions for portability (autotools
requires these here).
In my experience, autotools do not require them to be there or those
requirements can be relaxed as needed. Our portability wrappers may
still use Squid-specific code so placing them outside of src/ is not a
good idea, IMO. There got to be a better reason to place something
outside of src/ than autotools :-).
My reading of the AC_REPLACE_FUNCS(...) is that it when it detects a
portability issue it adds include/missing-function.h and
lib/missing-function.c to the build list.
We are using that for several library functions.
?somewhere? == third party additions (currently /lib/name/*
/lib/name is not too bad. We can even do src/3rdparty/name or something
like that, but perhaps keeping that stuff outside of src/ is a good idea
even though it is also source.
I'm not disagreeing on lib/name/. Just splitting the so its kept seperate
from the raw portability files.
/src/ == core code at lowest level
I would move it to src/base or src/backend or something like that. And
there should not be really many files there.
/src/common ?? (I think you were against a /src/core earlier)
/src/module == 'independant' module code. namely the files like
existing
ICAP, FS, Auth code. But making sure that all the other independant code
gets its own sub-dirs too (thinking each server-side gateway, pinger,
DNS?, ESI, Others?)
Yes, but module and independent should be treated loosely. For
example, logging may not be a module or independent, but may need
its own directory. Same for memory management.
In short, when you have or want to have a bunch of Something*.{cc,h}
files, place them into src/Something/ directory.
Exactly.
Amos
Re: two xasserts in squid3
On Mon, 2008-02-11 at 10:15 +1300, Amos Jeffries wrote: I think a better approach to this would be: 1) do we actually need it anyway? Yes, we need a common assert-like macro. 2) where is it supposed to be defined? IMO: include/xassert.h if xassert is used outside of src/ src/xassert.h otherwise, until we get something like src/base/ or src/misc for general-purpose commonly used things that do not fit anywhere else. I'm willing to commit time to that if we can agree on the 'old' files that need removing and the scope of the remining ones. I have seen a few things that made me think squid.h and defines.h may need to (merge?) stick around under a better definition of content. I would welcome such cleanup, especially after we switch to a VCS that does file renaming. IMO, there needs to be one header file like squid.h that is guaranteed to be the first one included by all other source files in src/. That file should only contain things needed by 99.9% of the code base. This is usually limited to hacks that change some global system or compiler effects. The rest should have their own purpose-specific header files. I was thinking that squid.h is the good choice for that. Just with much less tangled dependancy than it currently has. With some that is currently in defines.h. FWIW: I have a test run going over typedefs and managed to kill a few dozen lines of code there without side-effects on linux. I'll have it in cleanup branch soon for testing on other OS and compilers. If the source code needs something used by others, it needs to include something.h. There is also a question of whether some lib/ stuff needs to be moved to src/, but that is a separate debate and we need a better VCS for that as well. Some does, ie IPAddress I think should go back. My initial reason for placing it there is no longer valid. IMO Mem* and Debug shouls not be there either, but in wherever the core goes. Amos
Re: two xasserts in squid3
On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. When we get a better VCS, we should discuss moving include/ and lib/ stuff into src/ with the exception of 3rd party code. This would avoid problems created by that artificial boundary. What I have been mulling over after seeing code heirarchy like this: /include + /lib == C library functions for portability (autotools requires these here). ?somewhere? == third party additions (currently /lib/name/* /src/ == core code at lowest level /src/module == 'independant' module code. namely the files like existing ICAP, FS, Auth code. But making sure that all the other independant code gets its own sub-dirs too (thinking each server-side gateway, pinger, DNS?, ESI, Others?) Have I understood the earlier plans right? I think its a good structure, but as you say needs a better VCS to implement fully. Amos
Re: two xasserts in squid3
Alex Rousskov wrote: On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. It is not the best to have the declaration at library headers (include/*) and the definition in program sources, but yes looks that it is the safest solution at this time. Should I fix it in async-calls branch, or someone else will do the job in squid3-HEAD? I have yesterday created a branch cleanup in Squid-3 for the purpose of performing the larger cleanup operations instead of HEAD. This is so we can maintain better stability in HEAD and the bigger or more experimental changes can be done and tested in cleanup before going mainstream. Feel free to do it there and see what breaks. Amos The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. Are not exactly the same. In the Debug.h implementation there is an extra #if PURIFY . The assert.h file included at the begining of squid.h file and the Debug.h included in defines.h file which also included at the squid.h. It is not clear with a first view which assert called, and I wondering if there is a trick here..
Re: two xasserts in squid3
Alex Rousskov wrote: On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. It is not the best to have the declaration at library headers (include/*) and the definition in program sources, but yes looks that it is the safest solution at this time. Should I fix it in async-calls branch, or someone else will do the job in squid3-HEAD? The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. Are not exactly the same. In the Debug.h implementation there is an extra #if PURIFY . The assert.h file included at the begining of squid.h file and the Debug.h included in defines.h file which also included at the squid.h. It is not clear with a first view which assert called, and I wondering if there is a trick here..
Re: two xasserts in squid3
On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. When we get a better VCS, we should discuss moving include/ and lib/ stuff into src/ with the exception of 3rd party code. This would avoid problems created by that artificial boundary. Alex.
Re: two xasserts in squid3
On Mon, 2008-02-11 at 09:33 +1100, Robert Collins wrote: One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. Yes, of course. That would be the focus of the cleanup that Amos is volunteering to do :-) Alex.
Re: squid-3.HEAD IPAddress leak
On Fri, 2008-02-08 at 12:14 +0900, Adrian Chadd wrote: Together they make a pretty tree. But every used piece is eseentially another new, memset, free. Ah, and here you will have problems. The members of that struct should probably be malloc, free, and not new/delete. You're using new/delete which -should- map to a default new operator and head off to the malloc libraries, but -squids- idea of the malloc interface could differ from the -library- idea of the malloc interface. You can tell C++ to use malloc/free if needed. Am I reading right that the OS calls free itself ? Usually /either/ clients free and allocate, or libraries free and allocate - never a mix. (Because even in C the malloc libraries can be partially overridden). If squid is doing the allocation and free the following: So you should probably drop the new/delete'ing of the addrinfo stuff and replace it with malloc/free. is irrelevant, but if its shared then we should definitely not use C++ because operator delete can't be called. (And don't use talloc, or anything else either. You're also memset()'ing the addrinfo struct whether you allocated it or not, which may be double-memset'ing the thing, and if someone passed in an addrinfo it may have structure members which have now been leaked. tighter boundaries may help this. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
squid3 future directory structure
On Tue, 2008-02-12 at 11:19 +1300, Amos Jeffries wrote: On Tue, 2008-02-12 at 10:37 +1300, Amos Jeffries wrote: When we get a better VCS, we should discuss moving include/ and lib/ stuff into src/ with the exception of 3rd party code. This would avoid problems created by that artificial boundary. What I have been mulling over after seeing code heirarchy like this: /include + /lib == C library functions for portability (autotools requires these here). In my experience, autotools do not require them to be there or those requirements can be relaxed as needed. Our portability wrappers may still use Squid-specific code so placing them outside of src/ is not a good idea, IMO. There got to be a better reason to place something outside of src/ than autotools :-). My reading of the AC_REPLACE_FUNCS(...) is that it when it detects a portability issue it adds include/missing-function.h and lib/missing-function.c to the build list. We are using that for several library functions. Does AC_CONFIG_LIBOBJ_DIR control the name of the lib/ directory? Macro: AC_CONFIG_LIBOBJ_DIR (DIRECTORY) Specify that `AC_LIBOBJ' replacement files are to be found in DIRECTORY, a name relative to the top level of the source tree. The replacement directory defaults to `.', the top level directory, and the most typical value is `lib', corresponding to `AC_CONFIG_LIBOBJ_DIR([lib])'. `configure' might need to know the replacement directory for the following reasons: (i) some checks use the replacement files, (ii) some macros bypass broken system headers by installing links to the replacement headers (iii) when used in conjunction with Automake, within each makefile, DIRECTORY is used as a relative path from `$(top_srcdir)' to each object named in `LIBOBJS' and `LTLIBOBJS', etc. ?somewhere? == third party additions (currently /lib/name/* /lib/name is not too bad. We can even do src/3rdparty/name or something like that, but perhaps keeping that stuff outside of src/ is a good idea even though it is also source. I'm not disagreeing on lib/name/. Just splitting the so its kept seperate from the raw portability files. Right. FWIW, the dir/many-files-here* plus dir/many-subdirs-here/* combination always looks messy and awkward to search/navigate to me. I would prefer just dir/many-subdirs-here/* but it is not a big deal. /src/ == core code at lowest level I would move it to src/base or src/backend or something like that. And there should not be really many files there. /src/common ?? (I think you were against a /src/core earlier) Common is good enough to me as it reflects the purpose fairly well. Core would be wrong because this directory is for little things used throughout the project rather than some kind of a monolithic kernel holding everything together. BTW, we should not name that subdir core regardless of its purpose -- I have tried that in another project and got bitten by systems/programs that treat that name specially :-). Another big related question is the header path problem: Do we want to have something like squid/src/squid/module/*, which allows you to refer to Squid include files as squid/module/something.h as opposed to module/something.h or, horror, something.h? AFAIK, this arrangement is necessary if you: 1) want to install some of the header files from subdirs (e.g., for folks who build 3rd party eCAP modules); 2) do not want to pollute global header namespace with likely-to-clash file names like Log.h or select.h; and 3) do not want to keep all header files separately from .cc files, using the squid/include/module/something.h template (which I find much uglier and more awkward to use than squid/src/squid/module/ where related .h and .cc files are kept in one module/ directory). There may be better solutions to the header path problem, but I do not know them. HTH, Alex.
Re: two xasserts in squid3
On Tue, 2008-02-12 at 11:27 +1300, Amos Jeffries wrote: * Add automatic testing for header dependency - script to perform universal include unit-test for .h files - link to automatic unit-testing in each directory - fix the compile errors! I am thinking along the lines of unit-testing the compilation of each *.h file. At present a fvery few of the important .h files have a hard-coded .cc in the tests directory and that gets built when unit-testing. All it does is #include something.h What I'm planning is a test-suite script that when passed a directory, scans it for all .h files. Writes the .cc and attempts a build. Any compile errors are dependancy errors needing a fix. That would speed-track the dependancy testing in future with no need for manually adding these tests, or a large set of files for them. It is particularly useful for the struct and proto re-arranging. Sounds good. Thank you, Alex.
Re: two xasserts in squid3
On Tue, 2008-02-12 at 11:10 +1300, Amos Jeffries wrote: On Mon, 2008-02-11 at 09:33 +1100, Robert Collins wrote: One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. Yes, of course. That would be the focus of the cleanup that Amos is volunteering to do :-) Hows this for an action plan Robert? * Obsolete typedefs.h (underway) - remove all unneeded typedefs - move all needed typedefs to their appropriate headers - fix compile errors * Add automatic testing for header dependency - script to perform universal include unit-test for .h files - link to automatic unit-testing in each directory - fix the compile errors! * Obsolete protos.h - move all protos to their appropriate header files - add includes for headers where needed. * Obsolete structs.h - move all structs to their appropriate header files - move modular configuration in to *Config.h files (discussion on exactly what the modules are) I like the automatic include unit test. I found while slowly working through this that circular structure dependencies made it quite tricky; Yes found that already, and have not even started. ;-) I can't believe I'm thankful that many of them use pointer-fields. I think you will find you need * Break dependencies Please explain. I assume you mean the class Something; pre-definitions instead of #includes. to make it really clean, but that can be longer term if needed. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt.
