20080207-squid3-nonzero-buffers.diff
Adrian, A few comments regarding the nonzero patch you posted on IRC: http://www.creative.net.au/diffs/20080207-squid3-nonzero-buffers.diff First of all, it is a move in the right direction, of course. If you verified that it works, it can be committed as is. Here are a few nitpicks since you asked for comments: - Replace dontZeroMe() with zeroOnPush(bool doIt) because negative names are confusing and because we are not zeroing the object (this or me), but the memory we push into it. - Remove virtual from dontZeroMe(). Makes folks look hard for extensions, etc. - Make zeroBuffer private. Alternatively, make it public and remove the set method. Protected data members cause weird C++ problems, unfortunately. - Rename zeroBuffer to doZeroOnPush so that the association with the set method is clear and the purpose is more precise. - Remove memDataNonZero() if possible. It do nothing new and we need fewer globals. - Consider adding boolean aZeroOnPush parameter to memDataInit(), with false as the default value. Thank you, Alex.
a simple formatter
Hi all,
I wrote a small perl script which fixes (some of) the astyle problems.
Before use it, you must adjust the $ASTYLE_BIN variable at the beggining
of the formatter.pl file.
I am running it using the following command:
# find . -name *.cc -exec formater.pl \{\} \;
but it can take multiple files as arguments.
I used it with astyle versions 1.18 and 1.21. I did not do extensive
tests, so I am sure it contains bugs :-). Please use it with caution.
I tried to implement Alex suggestions. For the problem discussion look
at the thread:
http://www.squid-cache.org/mail-archive/squid-dev/200801/0019.html
This script starts the astyle and attach one filter in its input and one
in its output, using pipes.
The input filter search for unsigned int X:1; cases and convert
them to unsigned int X__FORASTYLE__1;.
The output filter search for unsigned int X__FORASTYLE__1;
patterns and convert them back to the original text.
Also it search for the cases #preprocessor directive { or {
#preprocessor_directive and fix them.
Do we need something like that? Any comments/suggestions? Any testers?
Regards,
Christos
formater.pl
Description: Perl program
Re: Solaris privileges to use pinger w/o setuid-root
On Sat, Jan 26, 2008 at 01:16:14PM +1300, Amos Jeffries wrote: Frank Fegert wrote: On Fri, Jan 25, 2008 at 09:25:24AM +1300, Amos Jeffries wrote: i did a quick hack and patched Solaris privileges support into pinger.c from squid-2.6.STABLE18. This should allow to run pinger w/o setuid-root, while still being able to access ICMP-sockets. The $SQUID_USER gets the additional PRIV_NET_ICMPACCESS rights via: /usr/sbin/usermod -K defaultpriv=basic,net_icmpaccess $SQUID_USER While probably not so interesting for the general public, could someone with a bit more squid-code knowledge than me take a look at the patch? I just want to make sure i didn't inadvertedly break something else ;-) Interesting and useful. Thank you. Thank you for your fast reply. I should add that i didn't invent the wheel here ;-) There is a quite nice documentation on the subject: http://docs.sun.com/app/docs/doc/816-4863/chap1-intro-net-01 so credits should go towards Sun ;-) Ah, in which case we have to ask: Is the code copyright available under GPL v2 and later Lisences? The copyright issues have been getting a bit of cleanup lately and we don't want to go backwards if possible. Good point, but (luckyly) IANAL. Sun indicates in the first page of the document mentioned above, that the information in the document is subject to Suns or a third partys copyright. Sound like a standard text blob, but does that also include work derived from the document? Regards, Frank
Re: a simple formatter
Hi all,
I wrote a small perl script which fixes (some of) the astyle problems.
Before use it, you must adjust the $ASTYLE_BIN variable at the beggining
of the formatter.pl file.
I am running it using the following command:
# find . -name *.cc -exec formater.pl \{\} \;
but it can take multiple files as arguments.
I used it with astyle versions 1.18 and 1.21. I did not do extensive
tests, so I am sure it contains bugs :-). Please use it with caution.
I tried to implement Alex suggestions. For the problem discussion look
at the thread:
http://www.squid-cache.org/mail-archive/squid-dev/200801/0019.html
This script starts the astyle and attach one filter in its input and one
in its output, using pipes.
The input filter search for unsigned int X:1; cases and convert
them to unsigned int X__FORASTYLE__1;.
The output filter search for unsigned int X__FORASTYLE__1;
patterns and convert them back to the original text.
Also it search for the cases #preprocessor directive { or {
#preprocessor_directive and fix them.
Do we need something like that? Any comments/suggestions? Any testers?
Regards,
Christos
Tested.
I have made a (small) file of the structures I've noticed past formatting
errors on. With a manual fix-up to what I think it should look like in
readable C++ under the published squid formatting description.
The output of your script does this:
--- astyle-test-cases.cc-original 2008-02-07 12:13:07.0 +1300
+++ astyle-test-cases.cc2008-02-07 12:19:14.0 +1300
@@ -9,10 +9,11 @@
*/
/// structures with bit-fields
+
struct t1 {
-unsigned int a:1;
-unsigned int b:1;
-unsigned int c:1;
+unsigned int a:1;
+unsigned int b:1;
+unsigned int c:1;
};
/// code-blocks inside definition protection
@@ -20,6 +21,7 @@
{
;
}
+
#endif
/// regular function declarations
@@ -30,6 +32,7 @@
}
/// template class explicit Instantiation for some compilers
+
template class ACLStrategisedHttpHeader*;
/// global template Instances with macro definition
NP: breaking the auto-docs away from the documented object is not that
good a thing to do.
Amos
Re: 20080207-squid3-nonzero-buffers.diff
Updated patch: http://www.creative.net.au/diffs/20080207-squid3-nonzero-buffers-2.diff doZeroOnPush can't be private as it then seems to be inaccessible from MemPool::push(). Adrian On Wed, Feb 06, 2008, Alex Rousskov wrote: Adrian, A few comments regarding the nonzero patch you posted on IRC: http://www.creative.net.au/diffs/20080207-squid3-nonzero-buffers.diff First of all, it is a move in the right direction, of course. If you verified that it works, it can be committed as is. Here are a few nitpicks since you asked for comments: - Replace dontZeroMe() with zeroOnPush(bool doIt) because negative names are confusing and because we are not zeroing the object (this or me), but the memory we push into it. - Remove virtual from dontZeroMe(). Makes folks look hard for extensions, etc. - Make zeroBuffer private. Alternatively, make it public and remove the set method. Protected data members cause weird C++ problems, unfortunately. - Rename zeroBuffer to doZeroOnPush so that the association with the set method is clear and the purpose is more precise. - Remove memDataNonZero() if possible. It do nothing new and we need fewer globals. - Consider adding boolean aZeroOnPush parameter to memDataInit(), with false as the default value. Thank you, Alex. -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: URIs for accelerators
Thanks, I'll try that out. I can see henrik's point; this *could* be solved by improving the documentation (and in any case, it needs it). However, I tend to like what I think this patch is going to do; it's the least surprising thing. Cheers, On 03/02/2008, at 6:55 AM, Adrian Chadd wrote: On Wed, Jan 23, 2008, Mark Nottingham wrote: Short version: When an accelerator, Squid tends to omit the port number from its idea of what the request-URI is. This screws up logging, helpers, acls, etc. See: http://www.squid-cache.org/bugs/show_bug.cgi?id=2192 Try: http://www.creative.net.au/diffs/20080203-squid2-default-accel-port.diff =completely= untested! Adrian -- Mark Nottingham [EMAIL PROTECTED]
Re: Solaris privileges to use pinger w/o setuid-root
On Sat, Jan 26, 2008 at 01:16:14PM +1300, Amos Jeffries wrote: Frank Fegert wrote: On Fri, Jan 25, 2008 at 09:25:24AM +1300, Amos Jeffries wrote: i did a quick hack and patched Solaris privileges support into pinger.c from squid-2.6.STABLE18. This should allow to run pinger w/o setuid-root, while still being able to access ICMP-sockets. The $SQUID_USER gets the additional PRIV_NET_ICMPACCESS rights via: /usr/sbin/usermod -K defaultpriv=basic,net_icmpaccess $SQUID_USER While probably not so interesting for the general public, could someone with a bit more squid-code knowledge than me take a look at the patch? I just want to make sure i didn't inadvertedly break something else ;-) Interesting and useful. Thank you. Thank you for your fast reply. I should add that i didn't invent the wheel here ;-) There is a quite nice documentation on the subject: http://docs.sun.com/app/docs/doc/816-4863/chap1-intro-net-01 so credits should go towards Sun ;-) Ah, in which case we have to ask: Is the code copyright available under GPL v2 and later Lisences? The copyright issues have been getting a bit of cleanup lately and we don't want to go backwards if possible. Good point, but (luckyly) IANAL. Sun indicates in the first page of the document mentioned above, that the information in the document is subject to Suns or a third partys copyright. Sound like a standard text blob, but does that also include work derived from the document? You mean the Book information page? my reading of that it mentions a lot of sources, OpenSource and others. Without mentioning any conditions of those sub-liscencing. Without specifically mentioning anything about rights of users or readers it does clearly deny any uses where the 'information contained within' might reach a mysterious listof poeple any countries the US don't like. Nasty. The way copyrights seem to have been done in squid. If you have generated the code from a non-copyright brief description we should be able to accept it as your community work. If you have duplicated code or pseudo algorithm, from a copyright or limited use source (I'm not sure if tha US export law counts) we need to be able to verify that source allows any re-working freely, with its explicit copyright added to the squid bunch. Amos
Re: squid-3.HEAD IPAddress leak
Amos, could you please poke the leak in IPAddress:GetAddrInfo a little? I'll give it another try. But don't hold your breath too long on that one. Did you see my note on it? I tracked that leak backwards from two segfaults to the connect() system call. Then I tracked the allocation from my init forwards to the connect() call. As far as I can tell when connect() returns OK, things work. But if connect() fails in any way the addrinfo pointer is immediately pointing somewhere in the system read-only memory with a leak lost. Kernel segfaults squid when it attempt to cleanup anywhere in the read-only areas. Amos Thanks, Adrian ==19601== ==19601== ERROR SUMMARY: 9353 errors from 5 contexts (suppressed: 8 from 2) ==19601== malloc/free: in use at exit: 36,429,514 bytes in 14,790 blocks. ==19601== malloc/free: 114,225 allocs, 99,435 frees, 115,499,321 bytes allocated. ==19601== For counts of detected errors, rerun with: -v ==19601== searching for pointers to 14,790 not-freed blocks. ==19601== checked 37,289,184 bytes. ==19601== ==19601== ==19601== 4,104 bytes in 4 blocks are possibly lost in loss record 10 of 27 ==19601==at 0x4C21C16: malloc (vg_replace_malloc.c:149) ==19601==by 0x4EDE17: xmalloc (util.c:506) ==19601==by 0x4754B8: httpHeaderBuildFieldsInfo (SquidNew.h:54) ==19601==by 0x46F44F: httpHeaderIdByNameDef (HttpHeader.cc:1707) ==19601==by 0x40E197: ACLHTTPHeaderData::parse() (ACLHTTPHeaderData.cc:90) ==19601==by 0x408D09: ACL::ParseAclLine(ConfigParser, ACL**) (acl.cc:153) ==19601==by 0x421365: parse_line(char*) (cache_cf.cc:888) ==19601==by 0x424F9F: parseOneConfigFile(char const*, unsigned) (cache_cf.cc:325) ==19601==by 0x425990: parseConfigFile(char const*, CacheManager) (cache_cf.cc:359) ==19601==by 0x483337: main (main.cc:1198) ==19601== ==19601== ==19601== 147,648 (110,736 direct, 36,912 indirect) bytes in 2,307 blocks are definitely lost in loss record 25 of 27 ==19601==at 0x4C21C16: malloc (vg_replace_malloc.c:149) ==19601==by 0x4EDE17: xmalloc (util.c:506) ==19601==by 0x4E9BD9: IPAddress::GetAddrInfo(addrinfo*, int) const (SquidNew.h:46) ==19601==by 0x4C2DAC: comm_connect_addr (comm.cc:1321) ==19601==by 0x4C3E70: ConnectStateData::connect() (comm.cc:1249) ==19601==by 0x44F7BA: FwdState::connectStart() (forward.cc:912) ==19601==by 0x4487AF: EventDispatcher::dispatch() (event.cc:131) ==19601==by 0x449747: EventLoop::runOnce() (EventLoop.cc:131) ==19601==by 0x449837: EventLoop::run() (EventLoop.cc:100) ==19601==by 0x4836F7: main (main.cc:1321) -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: 20080207-squid3-nonzero-buffers.diff
Updated patch: http://www.creative.net.au/diffs/20080207-squid3-nonzero-buffers-2.diff doZeroOnPush can't be private as it then seems to be inaccessible from MemPool::push(). Try setting up a friend' relationship between MemPool and MemAllocator classes. Or creating an inline accessor. Amos Adrian On Wed, Feb 06, 2008, Alex Rousskov wrote: Adrian, A few comments regarding the nonzero patch you posted on IRC: http://www.creative.net.au/diffs/20080207-squid3-nonzero-buffers.diff First of all, it is a move in the right direction, of course. If you verified that it works, it can be committed as is. Here are a few nitpicks since you asked for comments: - Replace dontZeroMe() with zeroOnPush(bool doIt) because negative names are confusing and because we are not zeroing the object (this or me), but the memory we push into it. - Remove virtual from dontZeroMe(). Makes folks look hard for extensions, etc. - Make zeroBuffer private. Alternatively, make it public and remove the set method. Protected data members cause weird C++ problems, unfortunately. - Rename zeroBuffer to doZeroOnPush so that the association with the set method is clear and the purpose is more precise. - Remove memDataNonZero() if possible. It do nothing new and we need fewer globals. - Consider adding boolean aZeroOnPush parameter to memDataInit(), with false as the default value. Thank you, Alex. -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: squid-3.HEAD IPAddress leak
On Thu, Feb 07, 2008, Amos Jeffries wrote: Amos, could you please poke the leak in IPAddress:GetAddrInfo a little? I'll give it another try. But don't hold your breath too long on that one. Did you see my note on it? I tracked that leak backwards from two segfaults to the connect() system call. Then I tracked the allocation from my init forwards to the connect() call. As far as I can tell when connect() returns OK, things work. But if connect() fails in any way the addrinfo pointer is immediately pointing somewhere in the system read-only memory with a leak lost. Kernel segfaults squid when it attempt to cleanup anywhere in the read-only areas. Come on IRC and we'll talk through it? I can't imagine its going to take long to fix. Adrian
Re: squid-3.HEAD IPAddress leak
On Thu, Feb 07, 2008, Amos Jeffries wrote: Amos, could you please poke the leak in IPAddress:GetAddrInfo a little? I'll give it another try. But don't hold your breath too long on that one. Why are you trying to allocate the structure on invocation of GetAddrInfo() ? Wouldn't it be better to simply assert the caller must provide storage for the structure, and you simply fill it in? That avoids a malloc/free pair (as it'll generally be in the stack frame of the caller or it'll be embedded in some other structure) and removes this special case allocation. Adrian -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: squid-3.HEAD IPAddress leak
On Thu, Feb 07, 2008, Amos Jeffries wrote: Amos, could you please poke the leak in IPAddress:GetAddrInfo a little? I'll give it another try. But don't hold your breath too long on that one. Did you see my note on it? I tracked that leak backwards from two segfaults to the connect() system call. Then I tracked the allocation from my init forwards to the connect() call. As far as I can tell when connect() returns OK, things work. But if connect() fails in any way the addrinfo pointer is immediately pointing somewhere in the system read-only memory with a leak lost. Kernel segfaults squid when it attempt to cleanup anywhere in the read-only areas. Argh, this temporary malloc/free pair is peppered throughout the codebase! Grr. I've removed that hack, and things work fine for me. Ubuntu 7.01 here, x86_64. Adrian
Re: squid-3.HEAD IPAddress leak
On Thu, 2008-02-07 at 11:56 +0900, Adrian Chadd wrote: Argh, this temporary malloc/free pair is peppered throughout the codebase! Grr. I've removed that hack, and things work fine for me. Ubuntu 7.01 here, x86_64. There's a useful c++ thing called 'placement new' - its used when you have classes that you want to use on the stack, but still have tight abstraction boundaries. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: squid-3.HEAD IPAddress leak
On Thu, Feb 07, 2008, Robert Collins wrote: I've removed that hack, and things work fine for me. Ubuntu 7.01 here, x86_64. There's a useful c++ thing called 'placement new' - its used when you have classes that you want to use on the stack, but still have tight abstraction boundaries. Well, I haven't removed the temporary malloc/free pair, whatever its called; I've just removed Amos' workaround in src/comm.cc so it doesn't leak on my system whilst I profile. Still, this is one of those death of a thousand cuts method of killing performance.. Adrian
Re: 20080207-squid3-nonzero-buffers.diff
On Thu, 2008-02-07 at 09:13 +0900, Adrian Chadd wrote: Updated patch: http://www.creative.net.au/diffs/20080207-squid3-nonzero-buffers-2.diff doZeroOnPush can't be private as it then seems to be inaccessible from MemPool::push(). Ouch. I did not realize the two ended up in different classes. doZeroOnPush has nothing to do with allocation and is not even used by MemAllocator, but it has to be there and not in MemPool because memPoolCreate macro returns MemImplementingAllocator and not a MemPool. Too much mess to untangle for this simple patch :-(. Let's commit the second version as is. Thank you, Alex. On Wed, Feb 06, 2008, Alex Rousskov wrote: Adrian, A few comments regarding the nonzero patch you posted on IRC: http://www.creative.net.au/diffs/20080207-squid3-nonzero-buffers.diff First of all, it is a move in the right direction, of course. If you verified that it works, it can be committed as is. Here are a few nitpicks since you asked for comments: - Replace dontZeroMe() with zeroOnPush(bool doIt) because negative names are confusing and because we are not zeroing the object (this or me), but the memory we push into it. - Remove virtual from dontZeroMe(). Makes folks look hard for extensions, etc. - Make zeroBuffer private. Alternatively, make it public and remove the set method. Protected data members cause weird C++ problems, unfortunately. - Rename zeroBuffer to doZeroOnPush so that the association with the set method is clear and the purpose is more precise. - Remove memDataNonZero() if possible. It do nothing new and we need fewer globals. - Consider adding boolean aZeroOnPush parameter to memDataInit(), with false as the default value. Thank you, Alex.
Re: 20080207-squid3-nonzero-buffers.diff
On Wed, Feb 06, 2008, Alex Rousskov wrote: Ouch. I did not realize the two ended up in different classes. yeah, neither did I until I started toying with it. doZeroOnPush has nothing to do with allocation and is not even used by MemAllocator, but it has to be there and not in MemPool because memPoolCreate macro returns MemImplementingAllocator and not a MemPool. Well, it means that it too can be used in the MemMalloc class as well as MemPool if one wished. Its just not done like that at the moment. Too much mess to untangle for this simple patch :-(. Let's commit the second version as is. Done. Adrian
Re: squid-3.HEAD IPAddress leak
On Thu, 2008-02-07 at 12:21 +0900, Adrian Chadd wrote: Well, I haven't removed the temporary malloc/free pair, whatever its called; I've just removed Amos' workaround in src/comm.cc so it doesn't leak on my system whilst I profile. Still, this is one of those death of a thousand cuts method of killing performance.. Right, I haven't seen the commit; care to mail the diff? -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: squid-3.HEAD IPAddress leak
On Thu, Feb 07, 2008, Robert Collins wrote:
Still, this is one of those death of a thousand cuts method of
killing performance..
Right, I haven't seen the commit; care to mail the diff?
Which? I just looked at the places where Amos is calling GetAddrInfo()
and FreeAddrInfo(); more then one are:
* GetAddrInfo(temp, );
* F-{something} = temp;
* FreeAddrInfo(temp);
--
- Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support -
- $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: squid-3.HEAD IPAddress leak
On Thu, Feb 07, 2008, Robert Collins wrote:
On Thu, 2008-02-07 at 14:08 +0900, Adrian Chadd wrote:
On Thu, Feb 07, 2008, Robert Collins wrote:
Still, this is one of those death of a thousand cuts method of
killing performance..
Right, I haven't seen the commit; care to mail the diff?
Which? I just looked at the places where Amos is calling GetAddrInfo()
and FreeAddrInfo(); more then one are:
* GetAddrInfo(temp, );
* F-{something} = temp;
* FreeAddrInfo(temp);
Wheee, surely that would be better as
F-{something} = GetAddrInfo(...)
or just GetAddrInfo(F-something, ...)
We're just filling in the details, there's no need to throw 50-60 bytes on
the stack and copy it when a pointer/reference will suffice.
Adrian
--
- Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support -
- $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
