Build failed in Hudson: 3.HEAD-i386-OpenBSD #576

[noc]

See 

Changes:

[Amos Jeffries ] Bug 3023: url_rewrite_program silently 
fails to rewrite on broken URLs

Produce loud cache.log entries displayig the URL instead of silently bypassing.

[Henrik Nordstrom ] Bug 3056 - comm.cc 
"!fd_table[fd].closing()" assertion from helperServerFree when a helper crashes 
while processing requests

reshuffle helperServerFree so it first unregisters the failed helper
and starts new ones if needed before it calls the callbacks on any
pending requests. If not those ends up resheduling the request on
this same crashed and partially shut down helper.

--
[...truncated 3116 lines...]
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in POP3
sed -e 's,[...@]perl[@],/usr/bin/perl,g' 
<../../../../helpers/basic_auth/POP3/basic_pop3_auth.pl.in >basic_pop3_auth || 
(/bin/rm -f -f basic_pop3_auth ; exit 1)
Making all in RADIUS
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT 
basic_radius_auth.o -MD -MP -MF ".deps/basic_radius_auth.Tpo" -c -o 
basic_radius_auth.o ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc; 
 then mv -f ".deps/basic_radius_auth.Tpo" ".deps/basic_radius_auth.Po"; else rm 
-f ".deps/basic_radius_auth.Tpo"; exit 1; fi
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT radius-util.o 
-MD -MP -MF ".deps/radius-util.Tpo" -c -o radius-util.o 
../../../../helpers/basic_auth/RADIUS/radius-util.cc;  then mv -f 
".deps/radius-util.Tpo" ".deps/radius-util.Po"; else rm -f 
".deps/radius-util.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_radius_auth  basic_radius_auth.o  radius-util.o -L../../../lib -lmiscutil 
 ../../../compat/libcompat.la-lm 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o 
radius-util.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a -lm
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
basic_radius_auth.o(.text+0x128): In function `main':
../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc:471: warning: 
strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in fake
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include   -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 
-MT fake.o -MD -MP -MF ".deps/fake.Tpo" -c -o fake.o 
../../../../helpers/basic_auth/fake/fake.cc;  then mv -f ".deps/fake.Tpo" 
".deps/fake.Po"; else rm -f ".deps/fake.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_fake_auth  fake.o -L../../../lib -lmiscutil  ../../../compat/libcompat.la 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in getpwnam
if ccache 

NULL vs 0

[Alex Rousskov]

On 09/20/2010 11:24 PM, Amos Jeffries wrote:

On Mon, 20 Sep 2010 17:44:17 -0600, Alex Rousskov wrote:

It is best to use 0 in C++ programs, but we have too much NULL-using
code to fight.



Easy enough to fix with a grep/sed. Do we make '0' a coding style
requirement?


Yes, we should if there are no objections. However, replacing current 
NULLs will invalidate most pending patches. I would not do it 
proactively to the old code.




The 'NULL not always 0' is relevant to Win32 builds with MS Visual Studio
which sets NULL == 0xCDCDCDCD (the kernels invalid RAM pattern, somewhere
out in invalid memory space). I'm not sure if the newer VS still do this.



Squid will most likely not work if NULL is not false. 0xCDCDCDCD is not 
false. Consider:


some_pointer = some_function_that_may_return_NULL();
if (!some_pointer)
...

Cheers,

Alex.

kids, workers, processes

[Alex Rousskov]

On 09/20/2010 11:24 PM, Amos Jeffries wrote:

On Mon, 20 Sep 2010 17:44:17 -0600, Alex Rousskov wrote:

You might be confusing processes with kids with workers, which is not
surprising given the evolving terminology and overlapping scopes.
Happens to me too, and I wrote or designed most of that code! Kids are
processes started by the Master Process. Watch_child in main.cc of the
Master Process is waiting for them. Coordinator process is one of the
kids.

Workers are Squid-like processes with no specialized function or powers.
They do what "squid -N" does, essentially. Depending on configuration,
there may be no kids, but there has to be at least one worker process.

Why the current code pieces seem correct in isolation, I think we will
change the terminology to something simpler and more coherent once we
have more experience with this stuff. Or perhaps it will feel more
coherent with time :-).


Now is good. Before too many people have played with it and got their
minds around the current naming.

How about:
  kid - "a" process started by Squid for doing "stuff".
  worker - a specialist kid performing HTTP request handling.
  coordinator - specialized process for managing a collection of kids


Currently, that does not account for

(a) no-daemon mode where there is a worker process but no kids. The 
worker is not a kid in this case.


(b) helper and other processes that are started by Squid for doing 
"stuff" but that are not kids (there is no corresponding Kid entry for 
them in the global Kids array managed by the Master Process).




For now we have a special case where coordinator==worker +
unlinkd/pinger/helper specialized kids.


Sorry, not sure what you mean by "coordinator==worker + ..."


Which may or may not be a good thing to carry on into the future, but
certainly simplifies debugging and testing.

If you agree on that the numberOf*() function would be closer to
numberOfWorkers() with no special cases needing mention. It scales to Kids
if you set the affinity for helpers as well.


Would not the no-daemon mode be always special? And if we make helpers 
into Kids (which is the right thing to do long-term), the no-daemon mode 
would be even more special because some kids will be started in 
no-daemon mode while others wont be.


Cheers,

Alex.

Re: StringNg review request

[Kinkie]

First of all, thanks.

On to the review :)

On Fri, Sep 17, 2010 at 1:25 PM, Alex Rousskov
 wrote:
> On 09/03/2010 09:21 AM, Kinkie wrote:
>
>> You'll find the branch at lp:~kinkie/squid/stringng
>> A few details on what to home on:
>> src/SBuf,* : the SBuf class proper
>
> The review for SBuf is below. The code is getting better overall, but there
> are still so many corrections that I will need more time to get the other
> classes reviewed.
>
> Review is based on lp:~kinkie/squid/stringng r9471.
>
> Original source code is prepended with "> ". The comments are below the
> source code they refer to. A single "." comment is there just to separate
> one piece of code from another.
>
>
> For SBuf.h:
>
>> #include "Debug.h"
>> #include "RefCount.h"
>
> Remove if not needed for SBuf.h.
> If SBuf.cci or any other file needs these, it should include them.

Fixed.

>> #include 
>
> Remove if not needed for SBuf.h

fixed.

>> #include 
>
> Use  instead if possible.

Done.

>> #include 
>
> Remove if not needed for SBuf.h. Does this need #define protection?

Done. Yes.

>> #define SQUIDSBUFPRINT(s) s.length(),s.exportRawRef()
>
> Missing parens around s.

It's not causing iesues, but won't hurt either. SquidString has the
same issue in trunk.

>> /**
>>  * Container for varioous SBuf class-wide statistics.
>
> spelling

fixed

>>  * \note read operations and compares are NOT counted.
>
> Remove. Many things are not counted.

ok

>
>> class SBufStats {
>>     public:
>
> It would be nice to run source formatting script against the branch.

Sure; Amos, could you point me at where to find it? Thanks!

>>     u_int64_t alloc; ///number of allocation operations
>
> Vague description: What is being allocated? SBuf? internal buffers?
> Consider: "number of constructor calls"

fixed.

> Here and elsewhere, doxygen comments are missing "<" to refer to the member
> on the left?

My ignorance, I didn't know the construct.

>>     u_int64_t live; ///number of free operations
>
> See above.
> Consider: "number of destructor calls"

fixed.

>>     u_int64_t qset; ///number of assigns/appends without content copy
>>     u_int64_t sset; ///number of assigns/appends with content copy
>
> I am not sure what the actual intent here is, but the description does not
> match use. qset is used in clear() and other methods unrelated to
> assigns/appends.

How about reworking the stats so that they actually count the
performed operations? (e.g. assign fast/slow, append fast/slow,
compare fast/slow etc.)?

>>     u_int64_t qget; ///number of get-ops not requiring full data scan
>>     u_int64_t sget; ///number of get-ops requiring a full data scan/copy
>
> It is not clear what "full data scan" is, why it is limited to get
> operations, and why the descriptions are not exactly symmetric.
> I did not review stats correctness because I do not know what most of these
> members really mean.

yes. see above.


>> /**
>>  * String-Buffer hybrid class (UniversalBuffer)
>
> Confusing to uninitiated.
> Consider: A string or buffer.

changed.

>>  * Features: refcounted backing store, cheap copy and substringing
>>  * operations, cheap-ish append operations, adaptive memory management
>
> Remove "cheap-ish append operations" because it is too vague to be useful
> and because there are no special append optimizations for now. At least I do
> not see them in this class and other classes should not matter here.

ok

> Remove "adaptive memory management" buzzwords.

ok

>>  * copy-on-write semantics to isolate change operations to each insatnce..
>
> spelling

fixed.

>>  * See http://www.cplusplus.com/reference/string/string/ for documentation
>> about
>>  * that API.
>
> Remove commercial site ad? Folks will find it if they need it.

done.

>>  * Notice that users MUST NOT hold pointers to SBuf, as doing so
>>  * moots the whole point of using refcounted buffers in the first place.
>>  */
>
> Remove as technically inaccurate. Besides, users MUST NOT do many things.

ok. How about adding some text discouraging pointers and references?
(const ref is ok unless non-const operations are needed)

>> class SBuf {
>>     public:
>>     typedef signed int size_type;
>
> I think we should support 3GB offsets and such. Could be handy for
> referencing memory cache areas without copying. Is there any reason to limit
> this to int? How about using size_t or ssize_t?

3Gb monolithic buffers are not a very good idea imvho (see the maxSize
tuneable). Given this, the best datatype is probably int32_t - helps
for printf-style portability.

>>     static const size_type npos = -1;
>
> Use static_cast<> to avoid dependence on size_type being signed. See
> std::string::npos.

done.

>>     ///  Flag used by search methods to define case sensitivity
>>     static const bool caseInsensitive = false;
>>     static const bool caseSensitive = true;
>
> Convert to enum?

What is the benefit? (my ignorance)

>>     /// Maximum size of a SBuf: 256Mb. Arbitrary, but should be sane.
>

Re: NULL vs 0

[Robert Collins]

On Wed, Sep 22, 2010 at 4:58 AM, Alex Rousskov
 wrote:
> Squid will most likely not work if NULL is not false. 0xCDCDCDCD is not
> false. Consider:
>
>    some_pointer = some_function_that_may_return_NULL();
>    if (!some_pointer)
>        ...

When compilers do that, they also translate the if expression appropriately.

But they are also meant to handle NULL vs 0 transparently in that case, AIUI.

-Rob

Re: NULL vs 0

[Alex Rousskov]

On 09/21/2010 12:49 PM, Robert Collins wrote:

On Wed, Sep 22, 2010 at 4:58 AM, Alex Rousskov
  wrote:

Squid will most likely not work if NULL is not false. 0xCDCDCDCD is not
false. Consider:

some_pointer = some_function_that_may_return_NULL();
if (!some_pointer)
...


When compilers do that, they also translate the if expression appropriately.
But they are also meant to handle NULL vs 0 transparently in that case, AIUI.


Scary but good to know, thank you. Looks like 0xCDCDCDCD, if used as a 
pointer, becomes false in that case!


Alex.

Build failed in Hudson: 3.HEAD-i386-OpenBSD #577

[noc]

See 

Changes:

[Amos Jeffries ] Author: Various Translators
Translations Update auto-save

[Amos Jeffries ] Bug 3023: url_rewrite_program silently 
fails to rewrite on broken URLs

Produce loud cache.log entries displayig the URL instead of silently bypassing.

--
[...truncated 3096 lines...]
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in POP3
sed -e 's,[...@]perl[@],/usr/bin/perl,g' 
<../../../../helpers/basic_auth/POP3/basic_pop3_auth.pl.in >basic_pop3_auth || 
(/bin/rm -f -f basic_pop3_auth ; exit 1)
Making all in RADIUS
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT 
basic_radius_auth.o -MD -MP -MF ".deps/basic_radius_auth.Tpo" -c -o 
basic_radius_auth.o ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc; 
 then mv -f ".deps/basic_radius_auth.Tpo" ".deps/basic_radius_auth.Po"; else rm 
-f ".deps/basic_radius_auth.Tpo"; exit 1; fi
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT radius-util.o 
-MD -MP -MF ".deps/radius-util.Tpo" -c -o radius-util.o 
../../../../helpers/basic_auth/RADIUS/radius-util.cc;  then mv -f 
".deps/radius-util.Tpo" ".deps/radius-util.Po"; else rm -f 
".deps/radius-util.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_radius_auth  basic_radius_auth.o  radius-util.o -L../../../lib -lmiscutil 
 ../../../compat/libcompat.la-lm 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o 
radius-util.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a -lm
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
basic_radius_auth.o(.text+0x128): In function `main':
../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc:471: warning: 
strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in fake
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include   -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 
-MT fake.o -MD -MP -MF ".deps/fake.Tpo" -c -o fake.o 
../../../../helpers/basic_auth/fake/fake.cc;  then mv -f ".deps/fake.Tpo" 
".deps/fake.Po"; else rm -f ".deps/fake.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_fake_auth  fake.o -L../../../lib -lmiscutil  ../../../compat/libcompat.la 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in getpwnam
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include   -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 
-MT basic_getpwnam_auth.o -MD -MP -MF ".deps/basic_getpwnam_auth.Tpo" -c -o 
basic_getpwnam_auth.o 
../../../../helpe

Build failed in Hudson: 3.HEAD-i386-OpenBSD #578

[noc]

See 

Changes:

[Automatic source maintenance ] SourceFormat 
Enforcement

[Alex Rousskov ] Protect SQUIDSTRINGPRINT 
macro argument from merging with macro body parts.

No runtime changes expected.

[Amos Jeffries ] Author: Various Translators
Translations Update auto-save

--
[...truncated 3097 lines...]
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in POP3
sed -e 's,[...@]perl[@],/usr/bin/perl,g' 
<../../../../helpers/basic_auth/POP3/basic_pop3_auth.pl.in >basic_pop3_auth || 
(/bin/rm -f -f basic_pop3_auth ; exit 1)
Making all in RADIUS
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT 
basic_radius_auth.o -MD -MP -MF ".deps/basic_radius_auth.Tpo" -c -o 
basic_radius_auth.o ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc; 
 then mv -f ".deps/basic_radius_auth.Tpo" ".deps/basic_radius_auth.Po"; else rm 
-f ".deps/basic_radius_auth.Tpo"; exit 1; fi
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT radius-util.o 
-MD -MP -MF ".deps/radius-util.Tpo" -c -o radius-util.o 
../../../../helpers/basic_auth/RADIUS/radius-util.cc;  then mv -f 
".deps/radius-util.Tpo" ".deps/radius-util.Po"; else rm -f 
".deps/radius-util.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_radius_auth  basic_radius_auth.o  radius-util.o -L../../../lib -lmiscutil 
 ../../../compat/libcompat.la-lm 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o 
radius-util.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a -lm
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
basic_radius_auth.o(.text+0x128): In function `main':
../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc:471: warning: 
strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in fake
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include   -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 
-MT fake.o -MD -MP -MF ".deps/fake.Tpo" -c -o fake.o 
../../../../helpers/basic_auth/fake/fake.cc;  then mv -f ".deps/fake.Tpo" 
".deps/fake.Po"; else rm -f ".deps/fake.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_fake_auth  fake.o -L../../../lib -lmiscutil  ../../../compat/libcompat.la 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in getpwnam
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include   -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 
-MT basic_getpwnam_auth.o -MD -MP -MF ".deps/basic_getpwnam_auth.Tpo" -c -o 
basic_getpwnam_auth.o 
../.

Re: StringNg review request: SBuf.h

[Alex Rousskov]

On 09/21/2010 11:29 AM, Kinkie wrote:

Review is based on lp:~kinkie/squid/stringng r9471.



#define SQUIDSBUFPRINT(s) s.length(),s.exportRawRef()


Missing parens around s.


It's not causing iesues, but won't hurt either.


It is a bug.



SquidString has the same issue in trunk.


Fixed.



 u_int64_t qset; ///number of assigns/appends without content copy
 u_int64_t sset; ///number of assigns/appends with content copy


I am not sure what the actual intent here is, but the description does not
match use. qset is used in clear() and other methods unrelated to
assigns/appends.


How about reworking the stats so that they actually count the
performed operations? (e.g. assign fast/slow, append fast/slow,
compare fast/slow etc.)?


My recommendation is to remove most of the stats until we know what we 
actually need. Leaving either (constructor, destructor) stats or alive 
instances stats may be a good idea. I do not see how the rest can be 
useful for performance analysis right now.


I do not insist on removing any stats though. You may have some 
well-justified needs for all of them. My only requirement is that all 
stats are correct (which is impossible without a good description).




  * Notice that users MUST NOT hold pointers to SBuf, as doing so
  * moots the whole point of using refcounted buffers in the first place.
  */


Remove as technically inaccurate. Besides, users MUST NOT do many things.


ok. How about adding some text discouraging pointers and references?
(const ref is ok unless non-const operations are needed)


I am against it. SBuf.h is not the right place for a "Good Coding 
Practices" tutorial and your attempts to summarize the rules in a couple 
of sentences are likely continue to fail because there are many cases 
and many exceptions. If you feel like writing about general rules for 
proper use of refcounted objects, add a RefCount.dox file.




class SBuf {
 public:
 typedef signed int size_type;


I think we should support 3GB offsets and such. Could be handy for
referencing memory cache areas without copying. Is there any reason to limit
this to int? How about using size_t or ssize_t?


3Gb monolithic buffers are not a very good idea imvho (see the maxSize
tuneable).


Why not?! When I am writing an mmapped shared-memory cache, I do not 
have the luxury of allocating small buffers. I may want to maximize the 
size of the segment to minimize the number of segments used. Whether 
that would be the best design is debatable, but it seems wrong to decide 
that debate by limiting the maximum SBuf size like that.




Given this, the best datatype is probably int32_t - helps
for printf-style portability.


You already have a printf-friendly methods. I doubt we need to limit the 
buffer size just because printf() is difficult to deal with, especially 
when new code should avoid printf().




 ///  Flag used by search methods to define case sensitivity
 static const bool caseInsensitive = false;
 static const bool caseSensitive = true;


Convert to enum?


What is the benefit? (my ignorance)


Groups common names together, allows missing switch() value checks, may 
allow for safer/aggressive compiler optimization, does not force you to 
make up constant values, assures unique values by default, disables bad 
behavior such as taking caseSensitive address, etc. Not a big deal in 
this context though.




  *
  * A SBuf can be empty, but can never be undefined. In other words
there is no equivalent
  * of NULL. defined/undefined semantics need to be implemented by the
caller.
  */


Remove: Avoid documenting what SBuf cannot be. If you insist on discussing
the non-existent APIs, move that discussion to the SBuf.dox file.


How about moving a simplified version of it that to the class documentation?
I have committed such an attempt.


You are still documenting negatives, which is counter-productive. I know 
what you mean by "can never be undefined" but a new developer will not 
know how to interpret that because there is no defined() function in C/C++.





 /** Constructor: import c-style string
  *
  * Create a new SBuf containing a COPY of the contents of the
  * c-string
  * \param S the c string to be copied
  * \param Ssize the size of S. If it is SBuf::npos or unspecified,
  *  S must be null-terminated


Remove Ssize parameter. You have too many sizes. If you want a two-parameter
conversion from c-string, define another constructor with just two
parameters. See std::string.


std::string API does not cover specifying pos and length when
importing c-strings.
While I agree that mimicking that API is good, isn't this variant more
effective than copy-all-then-substring (or even worse
manipulate-in-c-then-import) ?


I am not against specifying pos and n when importing. I am against 
specifying size, pos, and n. The size argument is not needed (and you 
made mistakes when trying to tie all three togethe

Build failed in Hudson: 3.HEAD-i386-OpenBSD #579

[noc]

See 

Changes:

[Automatic source maintenance ] SourceFormat 
Enforcement

--
[...truncated 3094 lines...]
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in POP3
sed -e 's,[...@]perl[@],/usr/bin/perl,g' 
<../../../../helpers/basic_auth/POP3/basic_pop3_auth.pl.in >basic_pop3_auth || 
(/bin/rm -f -f basic_pop3_auth ; exit 1)
Making all in RADIUS
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT 
basic_radius_auth.o -MD -MP -MF ".deps/basic_radius_auth.Tpo" -c -o 
basic_radius_auth.o ../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc; 
 then mv -f ".deps/basic_radius_auth.Tpo" ".deps/basic_radius_auth.Po"; else rm 
-f ".deps/basic_radius_auth.Tpo"; exit 1; fi
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include
-I../../../../helpers/basic_auth/RADIUS-Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 -MT radius-util.o 
-MD -MP -MF ".deps/radius-util.Tpo" -c -o radius-util.o 
../../../../helpers/basic_auth/RADIUS/radius-util.cc;  then mv -f 
".deps/radius-util.Tpo" ".deps/radius-util.Po"; else rm -f 
".deps/radius-util.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_radius_auth  basic_radius_auth.o  radius-util.o -L../../../lib -lmiscutil 
 ../../../compat/libcompat.la-lm 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_radius_auth basic_radius_auth.o 
radius-util.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a -lm
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
basic_radius_auth.o(.text+0x128): In function `main':
../../../../helpers/basic_auth/RADIUS/basic_radius_auth.cc:471: warning: 
strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in fake
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include   -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 
-MT fake.o -MD -MP -MF ".deps/fake.Tpo" -c -o fake.o 
../../../../helpers/basic_auth/fake/fake.cc;  then mv -f ".deps/fake.Tpo" 
".deps/fake.Po"; else rm -f ".deps/fake.Tpo"; exit 1; fi
/bin/sh ../../../libtool --tag=CXX --mode=link ccache g++ -Wall -Wpointer-arith 
-Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT  -g -O2  -g -o 
basic_fake_auth  fake.o -L../../../lib -lmiscutil  ../../../compat/libcompat.la 
libtool: link: ccache g++ -Wall -Wpointer-arith -Wwrite-strings -Wcomments 
-Werror -pipe -D_REENTRANT -g -O2 -g -o basic_fake_auth fake.o  
-L
 -lmiscutil ../../../compat/.libs/libcompat.a
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: vsprintf() is often misused, please use vsnprintf()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcpy() is almost always misused, please use strlcpy()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: strcat() is almost always misused, please use strlcat()
/usr/local/lib/gcc/i386-unknown-openbsd4.7/4.2.4/../../../libestdc++.so.11.0: 
warning: sprintf() is often misused, please use snprintf()
Making all in getpwnam
if ccache g++ -DHAVE_CONFIG_H  -I../../../.. -I../../../../include 
-I../../../../src  -I../../../include -I/usr/local/include   -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments -Werror -pipe -D_REENTRANT -g -O2 
-MT basic_getpwnam_auth.o -MD -MP -MF ".deps/basic_getpwnam_auth.Tpo" -c -o 
basic_getpwnam_auth.o 
../../../../helpers/basic_auth/getpwnam/basic_getpwnam_auth.cc;  then mv -f 
".deps/basic_getpwnam_auth.Tpo" ".deps/basic_getpwnam_auth.Po"; else rm -f 
".deps/basic_getpwnam_auth.Tpo"; exit 1; fi
/bin/sh 

Re: StringNg review request: SBuf.h

[Amos Jeffries]

On Tue, 21 Sep 2010 19:25:37 -0600, Alex Rousskov
 wrote:
> On 09/21/2010 11:29 AM, Kinkie wrote:
>>> Review is based on lp:~kinkie/squid/stringng r9471.
>>

> 
  ///  Flag used by search methods to define case sensitivity
  static const bool caseInsensitive = false;
  static const bool caseSensitive = true;
>>>
>>> Convert to enum?
>>
>> What is the benefit? (my ignorance)
> 
> Groups common names together, allows missing switch() value checks, may 
> allow for safer/aggressive compiler optimization, does not force you to 
> make up constant values, assures unique values by default, disables bad 
> behavior such as taking caseSensitive address, etc. Not a big deal in 
> this context though.
> 

... can be defined in .h symbols without requiring special .cc global.

We also have src/mk-string-arrays.awk to turn enumName.h files into
enumName.cc containing const char* enumName_Str[] arrays for debugging. see
src/Makefile.am for usage

> 
>>> It would be better to avoid bare "this" in the above comments. It is
>>> awkward
>>> to read.
>>
>> I can't think of any better way to call it. I've removed the
>> reference, but I'm not sure it's clearer.
>> Copy-paste:
>>  /** compare to other SBuf, strcmp-style
>>   *
>>   * Compare two SBufs. behaves like strcmp(3) or strncmp(3)
depending
>>   * on the value of the case_sensitive parameter
> 
> Avoid repetition. You already document what the parameter does, below. 
> And I think you meant strcasecmp(3). Consider:
> 
> /** compare to another SBuf, strcmp-style


NOTE: If you have two distinct methods/objects needing identical long
descriptions doxygen offers the \see directive for de-duping local
descriptions.

  /** Copy SBuf contents into user-supplied C buffer.
   *
   * Export a copy of the SBuf's contents into the user-supplied
   * buffer, up to the user-supplied-length.
   * The exported buffer is guarranteed to have a terminating \0,
   but
   * may truncate contents if the SBuf is too big.
   * \return 0 if all is OK
   * \return num the number of actually-copied chars, INCLUDING
the
   \0
   * \note uses xstrncpy as low-level function
   */
  size_type copy(char *buf, size_type buflen) const;
>>>
>>> Do we really need to zero-terminate the above? This low-level method
>>> may be
>>> handy for I/O which does not require zero-termination. We should
>>> minimize
>>> the number of APIs that terminate, IMO.
>>
>> Zero-termination is performed by xstrncpy(), AFTER the copy is done.
>> It doesn't cow().
>> Should I mention this in the documentation instead?
> 
> My point is quite different. Do we really need to zero-terminate the 
> copied data? I suspect that we do not and should not.
> 
> The fact that your implementation currently does terminate because it 
> uses xstrncpy() is irrelevant.
> 
> Moreover, your implementation is buggy because xstrncpy does not 
> guarantee a full copy for binary data. Our xstrncpy also writes beyond 
> buflen and the standard strncpy does not guarantee termination. Get us 
> out of this snake pit!
> 
> The correct implementation should probably use memcpy().

Absolutely. memcpy of min(buflen, SBuf::size()).

Termination (if actually needed) in such a copy would be at the
explicitly-terminated point or potentially earlier for binary data
containing \0.

> 
> I am guessing we do not care about overlapping regions in this specific 
> method because copying from SBufs to SBuf is only possible if the two 
> have different MemBlobs. The implementation should document this 
> assumption or even assert it.

side tracked? above says you are discussing "Copy SBuf contents into
user-supplied C buffer".


 int SBuf::plength() const
>>>
>>> Do we need to place the return type on its own line in .cci?
>>
>> Should get fixed by running the formatter tool.
> 
> AFAIK, the formatter tool does not fix this, unfortunately. I see 
> Polygraph and other copied code in Squid that is not formatted correctly

> because of this.

Yes. The formatter does not fix this. It's one of my remaining annoyances.
That and the inconsistent spacings between function name and (.

Kinkie:
 as for your earlier question scripts/source-maintenance.sh does all the
automated enforcement.
 The formater.pl does not adequately check astyle version so can lead to
big trouble if you don't have 1.22 installed. If in doubt run it on a
master.squid-cache.org checkout.


 void SBuf::clear()
 {
 #if 0
  //enabling this code path, the store will be freed and
  reinitialized
  store_=getStorePrototype(); //uncomment to actually free storage
  upon
 clear()
 #else
  //enabling this code path, we try to release the store without
 deallocating it.
  // will be lazily reallocated if needed.
  if (store_->RefCountCount() == 1)
  store_->bufUsed=0;
>>>
>>

Re: StringNg review request: SBuf.h

[Alex Rousskov]

On 09/21/2010 10:00 PM, Amos Jeffries wrote:

I am guessing we do not care about overlapping regions in this specific
method because copying from SBufs to SBuf is only possible if the two
have different MemBlobs. The implementation should document this
assumption or even assert it.


side tracked? above says you are discussing "Copy SBuf contents into
user-supplied C buffer".


This is relevant. The user-supplied buffer can be SBuf::buf() or 
similar. So Amos found another fix: We should remove "C" from "C buffer" 
in copy() description as misleading.


Thank you,

Alex.

Re: StringNg review: MemBlob

[Alex Rousskov]

On 09/20/2010 08:18 PM, Alex Rousskov wrote:

On 09/03/2010 09:21 AM, Kinkie wrote:


You'll find the branch at lp:~kinkie/squid/stringng

...

Comments for MemBlob.cci r9472:


Found one more:


 * \note memcpy is used as the copying function. This is safe,
 *   because it is guarranteed by design that the copied-to
 *   memory area is only owned by the appended-to string,
 *   and thus doesn't overlap with the appended-from area
 */
void
MemBlob::append(const char *S, const MemBlob::size_type Ssize)
{
memcpy(mem+bufUsed,S,Ssize);
bufUsed+=Ssize;
++stats.append;
}


The \note is correct, but we should not mention "string" in MemBlob.cci. 
MemBlob does not (or should not) know what its users are. There may be a 
better way to express the same thought. Consider:


\none memcpy() is safe because we copy to an unused area

The note belongs to the append() implementation, not its description.


Thank you,

Alex.

Re: StringNg review: MemBlob

[Amos Jeffries]

On 22/09/10 16:31, Alex Rousskov wrote:

On 09/20/2010 08:18 PM, Alex Rousskov wrote:

On 09/03/2010 09:21 AM, Kinkie wrote:


You'll find the branch at lp:~kinkie/squid/stringng

...

Comments for MemBlob.cci r9472:


Found one more:


* \note memcpy is used as the copying function. This is safe,
* because it is guarranteed by design that the copied-to
* memory area is only owned by the appended-to string,
* and thus doesn't overlap with the appended-from area
*/
void
MemBlob::append(const char *S, const MemBlob::size_type Ssize)
{
memcpy(mem+bufUsed,S,Ssize);
bufUsed+=Ssize;
++stats.append;
}


The \note is correct,


No. As you pointed out to me earlier with SBuf the S here may be the 
result of:

  a.clear(); a.append(a.mem, 1);

...



but we should not mention "string" in MemBlob.cci.
MemBlob does not (or should not) know what its users are. There may be a
better way to express the same thought. Consider:

\none memcpy() is safe because we copy to an unused area


*that* is better. There is still no guarantee it wont overflow on the 
destination. memcpy() makes no mention about when happens when one of 
the pointers is NULL, but experience shows a lot of segfaults.


memcpy() docs state "always copies exactly num bytes" and that the 
buffers should not overlap.


memmove() is the safe one which should be used if there is any doubt 
about overlapping memory. But even that can't validate against overflows.




The note belongs to the append() implementation, not its description.



Amos
--
Please be using
  Current Stable Squid 2.7.STABLE9 or 3.1.8
  Beta testers wanted for 3.2.0.2