[stringng patch] removes terminate() method from SBuf

[Amos Jeffries]

see IRC discussions 2010-09-17 for the reasons.

Boils down to it not actually being needed and causing semantic problems 
when provided by SBuf().


Amos
=== modified file 'src/SBuf.cc'
--- src/SBuf.cc	2010-09-14 21:53:55 +
+++ src/SBuf.cc	2010-09-17 07:09:19 +
@@ -431,18 +431,6 @@
 return stats;
 }
 
-void SBuf::terminate()
-{
-if (!store_->canAppend(bufEnd(),1)) {
-grow(length()+1);
-++stats.sset;
-} else {
-++stats.qset;
-}
-*(bufEnd())='\0';
-++store_->bufUsed; //hand-appending, without recording the increased SBuf length.
-}
-
 SBuf& SBuf::absorb(char * cstr, SBuf::size_type len)
 {
 assign(cstr,len);
@@ -485,7 +473,15 @@
 
 char *SBuf::exportTRef()
 {
-terminate();
+if (!store_->canAppend(bufEnd(),1)) {
+grow(length()+1);
+++stats.sset;
+} else {
+++stats.qset;
+}
+*(bufEnd())='\0';
+++store_->bufUsed; //hand-appending, without recording the increased SBuf length.
+
 return buf();
 }
 
@@ -664,8 +660,7 @@
 va_list arg;
 int rv;
 va_start (arg, format);
-terminate();
-rv = vsscanf(buf(),format,arg);
+rv = vsscanf(exportTRef(),format,arg);
 va_end(arg);
 return rv;
 }

=== modified file 'src/SBuf.h'
--- src/SBuf.h	2010-09-10 06:43:54 +
+++ src/SBuf.h	2010-09-17 07:09:48 +
@@ -59,7 +59,7 @@
  * \note read operations and compares are NOT counted.
  */
 class SBufStats {
-public:
+public:
 u_int64_t alloc; ///number of allocation operations
 u_int64_t live; ///number of free operations
 u_int64_t qset; ///number of assigns/appends without content copy
@@ -88,7 +88,7 @@
  * moots the whole point of using refcounted buffers in the first place.
  */
 class SBuf {
-public:
+public:
 typedef signed int size_type;
 static const size_type npos = -1;
 
@@ -306,13 +306,6 @@
  */
 static const SBufStats& getStats();
 
-/** Null-terminate the SBuf
- *
- * Make sure that the first byte AFTER the SBuf is a NULL.
- * Doesn't alter the SBuf contents, may trigger a backing store reloaction
- */
-void terminate();
-
 /** Import a c-style string into the memory-managed world
  *
  * The char* MUST have been dynamically allocated via new,

Re: StringNg review request

[Alex Rousskov]

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.


> #include 

Remove if not needed for SBuf.h


> #include 

Use  instead if possible.


> #include 

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


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

Missing parens around s.


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

spelling

>  * \note read operations and compares are NOT counted.

Remove. Many things are not counted.

> class SBufStats {
> public:

It would be nice to run source formatting script against the branch.


> u_int64_t alloc; ///number of allocation operations

Vague description: What is being allocated? SBuf? internal buffers?
Consider: "number of constructor calls"

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



> u_int64_t live; ///number of free operations

See above.
Consider: "number of destructor calls"


> 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.



> 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.



> /**
>  * String-Buffer hybrid class (UniversalBuffer)

Confusing to uninitiated.
Consider: A string or buffer.


>  * 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.


Remove "adaptive memory management" buzzwords.


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


spelling


>  * 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.


>  * 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.


> 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?



> static const size_type npos = -1;

Use static_cast<> to avoid dependence on size_type being signed. See 
std::string::npos.



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

Convert to enum?

> /// Maximum size of a SBuf: 256Mb. Arbitrary, but should be sane.
> /// \note only checked when the SBuf needs to be extended, the 
first alloc can exceed it

> static const size_type maxSize=0xfff;

Remove "the first alloc can exceed it". Let's be consistent. Besides, 
the user does not control when things get allocated and does not know 
what "first alloc" is.



> /** Default constructor. Creates a new empty SBuf.

Do not document that a default constructor is a default constructor. 
Same for all other standard methods: constructors, destructors, operators.

Can you _create_ something _old_?

Replace with: /** Creates an empty SBuf.

>  *
>  * 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.



> /** Constructor: import c-style string
>  *
>  * Create a new SB

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

[noc]

See 

Changes:

[Amos Jeffries ] More manuals documentation updates

[Alex Rousskov ] Author: Edward Chernenko 

Fixed "stratup" typo in the auth_param examples.

--
[...truncated 3099 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/b

Re: StringNg review request

[Alex Rousskov]

On 09/17/2010 05:25 AM, Alex Rousskov wrote:

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

 > SBuf& SBuf::append(const SBuf &S)
 > {
 > debugs(SBUF_DEBUGSECTION,7,
 > MYNAME << "this=" << (void *)this <<
 > "appending=" <<(void *)&S<<")");
 > if (!store_->canAppend(bufEnd(),S.length())) {
 > grow(length()+S.length());
 > }

Please remove all such canAppend() guards. Just call grow(). It is much
safer than risking to forget a guard. Your grow() should not reAlloc if
growth is not needed. You may rename grow() to assureSpace() or similar
if you prefer but do not split the growth need check and the realloc
call in the "user" code.


The standard name for "make sure we have enough space" method is 
"reserve". See std::string::reserve() documentation for what it may and 
must do. Use it instead of the canAppend+grow duet.


Thank you,

Alex.

Re: [stringng patch] removes terminate() method from SBuf

[Alex Rousskov]

On 09/17/2010 01:19 AM, Amos Jeffries wrote:

see IRC discussions 2010-09-17 for the reasons.

Boils down to it not actually being needed and causing semantic problems
when provided by SBuf().


I would not remove terminate() completely because it does provide needed 
functionality. I would just make it private, document non-persistency 
caveat, and call it from c_str() and other applicable methods. If it is 
only used once, this becomes less important, of course, but I would 
still keep the code isolated.


Cheers,

Alex.

Re: Caching huge files in chunks?

[Alex Rousskov]

On 09/16/2010 06:21 PM, Guy Bashkansky wrote:

Here is the problem description, what solution might Squid or other
cache tools provide?

Some websites serve huge files, usually movies or binary distributions.
Typically a client issues byte range requests, which are not cacheable
as separate objects in Squid.
Waiting for the whole file to be brought into the cache takes way too
long, and is not granular enough for optimizations.

A possible solution would be if Squid (or other tool/plugin) knew how
to download huge files *in chunks*.
Then the tool would cache these chunks and transform them into
arbitrary ranges when serving client requests.
There are some possible optimizations, like predictive chunk caching
and cold chunks eviction.

Does anybody know how to put together such solution based on any existing tools?


Caching of partial responses is allowed by HTTP/1.1 but is not yet 
supported by Squid. It is a complex feature which can, indeed, be a 
useful optimization in some environments. For more information, please see


http://wiki.squid-cache.org/Features/PartialResponsesCaching


http://wiki.squid-cache.org/SquidFaq/AboutSquid#How_to_add_a_new_Squid_feature.2C_enhance.2C_of_fix_something.3F

Thank you,

Alex.

Re: [RFC] helper API

[Alex Rousskov]

On 09/16/2010 11:03 PM, Amos Jeffries wrote:

On 17/09/10 10:18, Alex Rousskov wrote:

On 09/16/2010 02:46 AM, Amos Jeffries wrote:

After the helper C++ build migration we have a partial API for the
helper tools. Some of them even make use of the #defined macros.

I managed to bungle and put the old-style debug() definition for helpers
into the libcompat. It's now clear that this would be better suited in
the API for helpers and non-squid tools.


What I'm looking at right now for the helpers is:
* some wrapper for main() that calls out to user functions for handling
a line received and processing command line options.

> * some definition of the user functions required to do the above.

If I understand what a helper is, and helper API should not wrap main or
standardize command line options processing. Main should be left for the
helper to control and there are probably enough libraries out there for
options parsing.

We can standardize a subset of common command line options, of course.

If you want to offer a helper development framework with such things as
built-in I/O and reconfiguration handling, then things change. You may
indeed take main() ownership and just leave some places for users to
plug their code into. I would not classify that as API though because it
restricts the implementation way beyond a common API needs.

And if we are doing a framework, we can remove current limitations
related to handling helper's standard input/output. We can just use
sockets...


Yes. Framework sounds more like what I'm asking about.

You mean provide a call to fetch a socket needed to send/receive from
Squid. Hiding the current std* ones behind the call until some future
re-work finds a need to change it?



If we are doing a Framework, then the user code should not know anything 
about sockets or other low-level ways to communicate with Squid. The 
code will integrate using something like noteSquidRequest() and 
provideResponseToSquid() API. If we do not want to switch to sockets 
now, we can do it later, transparently to the helper code.




* some macros (as now) for performing OK/ERR etc feedback to squid.
These take a char* parameter for additional key-pairs or messages.


By "performing feedback", do you mean actually writing bytes to the
stdout stream? Or just formatting the output for the helper to write. Do
you want to support helpers that want to manage I/O on their own? Do you
want to support embedded low-overhead helpers, eCAP-style?


At the moment its just this:
#define SEND_OK(x) fprintf(stdout,"OK %s\n", (x))


From API point of view, this implies that the Framework is managing 
I/O. There is not much management here, of course, but that is not 
important.



helpers may use the wrapper for convenience and easier to read code.
I've converted several of the simple helpers over already. But they are
still free to use the direct socket/pipe feedback if they need/want.


If needed, the Framework can expose the descriptor or class for writing 
responses but it will become more complex and rigid if we allow such raw 
access. I do not know whether it is needed. I am not a helper expert.




* the debug() call doing printf-style output as now but with automatic
prefixing of helper name and PID (matching the kidN for cache.log)


Printf() is far from ideal for C++ helpers. I do not know whether we
have or expect any though.


We are on the border. The helpers are C code built with g++ now. Future
ones could be pure C++.


Again, I do not know whether supporting C++ helpers is important. This 
is something you and others should decide. If it is, and if current 
helpers already compile with g++, I would not use printf.




* standardizing the -d (debug on) and -h (help) parameters for all
helpers compiled.




Does anyone have any advice about good ways to make a formal public API
that the existing bundled helpers, and potentially third-parties could
use when building C/C++ helpers for Squid?

ie things that must be one for versioning alterations over time.


I would start with defining what a helper is, and what you want to
provide helper authors/code with: A simple API/library their programs
would use, a framework where they can just plugin their custom stuff and
that would do I/O and process management for them, eCAP-like integration
API for embedded helpers, something else? You probably know all these
answers, but they are not obvious to some of us :-).



Definition (as per status-quo):
A third-party program for use by auth, external ACL, url/store/location
rewrite, and daemon logging. One which is started by Squid automatically
and processes requests received from Squid.


Three questions based on your definition:

1) If you want to provide a Framework, why limit helpers to programs? We 
can define a helper as "code implementing required Framework 
functionality for use by ..." and leave open the possibility of embedded 
helpers (without necessarily immediately implementing it, of course).


In gen

Re: Patch to add netfilter mark support

[Andrew Beverley]

On Thu, 2010-09-16 at 16:23 -0600, Alex Rousskov wrote:
> On 09/15/2010 12:12 AM, Andrew Beverley wrote:
> > On Wed, 2010-09-15 at 02:06 +, Amos Jeffries wrote:
> >> On Tue, 14 Sep 2010 23:55:20 +0100, Andrew Beverley
> >> wrote:
>    * Config.accessList.outgoingTos, Config.accessList.clientsideTos,
>  Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark
> >> can
>  become members of the Qos scope Config object. All the parsing /free
>  stuff
>  can be moved there too with some #define parse_...() etc for the legacy
>  parser.
> 
> >>>
> >>> I've moved all the configuration variables and functions to the Qos
> >>> scope. I have renamed parse_acl_tos(acl_tos ** head) as
> >>> Ip::Qos::Config::parseConfigAclTos(acl_tos ** head).
> >>>
> >>> However, I'm unable to compile because of the following error:
> >>>
> >>> Qos.cc: In member function ‘void
> >>> Ip::Qos::Config::parseConfigAclTos(acl_tos**)’:
> >>> Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’
> >> does
> >>> not match ‘void (*)(void*)’
> >>>
> >>> The code at line 377 is:
> >>>
> >>> CBDATA_INIT_TYPE_FREECB(acl_tos, freedConfigAclTos);
> >>>
> >>> I have
> >>>
> >>> CBDATA_TYPE(acl_tos);
> >>>
> >>> specified before the parseConfigAclTos function.
> >>>
> >>> Could you give me any ideas as to what I am doing wrong here? If you
> >>> need me to send through any more of the code then please let me know.
> >>
> >> Do you have this with a cast?
> >>   #define parse_acl_tos(X) Ip::Qos::Config::parseConfigAclTos((acl_tos
> >> **)X)
> >>
> >> with the cf.data.pre "TYPE: acl_tos" unchanged.
> >
> > No, I had changed it. However, I have now changed it back to the above,
> > but still get the same error. Any other ideas?
> >
> > Qos.cc: In member function ‘void 
> > Ip::Qos::Config::parseConfigAclTos(acl_tos**)’:
> > Qos.cc:377: error: argument of type ‘void (Ip::Qos::Config::)(void*)’ does 
> > not match ‘void (*)(void*)’
> >
> > I have attached my current Qos.cc and Qos.h
> 
> I kind of lost track of the current state. Do you still want help with 
> making this work? Or did you give up and are now back to the old C-style 
> craft outside of the proper namespace?

I gave up and moved the config functions (for outgoing_tos etc) back to
cache_cf.cc. The original QOS config parsing is still in Qos.cc. I
thought from what Amos said that it couldn't be made to work with
CBDATA, but if you have any suggestions then I'll happily look at them.

I'll hopefully send the patch in its current format through later on
today anyway, so you can see its current state.

Thanks,

Andy

Re: [stringng patch] removes terminate() method from SBuf

[Amos Jeffries]

On 18/09/10 05:33, Alex Rousskov wrote:

On 09/17/2010 01:19 AM, Amos Jeffries wrote:

see IRC discussions 2010-09-17 for the reasons.

Boils down to it not actually being needed and causing semantic problems
when provided by SBuf().


I would not remove terminate() completely because it does provide needed
functionality. I would just make it private, document non-persistency
caveat, and call it from c_str() and other applicable methods. If it is
only used once, this becomes less important, of course, but I would
still keep the code isolated.


The way I look at it exportTRef()** as it exists today can be used 
internally to implement terminate(). This patch alters the (one) use of 
terminate to do exactly that.
 Whether the post-transition code keeps exportTRef()** privately for 
this purpose is outside the scope.


** current name used for clarity.

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

Re: Updated netfilter mark patch

[Amos Jeffries]

On 18/09/10 09:18, Andrew Beverley wrote:

Hi,

Please find attached updated netfilter mark (and QOS tidy up) patch.

It takes into account all the recent feedback, but leaves the
tcp_outgoing_* and clientside_* configuration functions in cache_cf.cc
as discussed on the mailing list.

It remains not fully tested, but is provided for any further comments.

Thanks,

Andy




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