SqString

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

SqString

Henrik Nordström
Should we back out SqString for now until the implicit cast issues have
been analyzed in more detail, or try fixing it somehow for the 3.0
release?

http://www.squid-cache.org/bugs/show_bug.cgi?id=1970

My vote is to defer the SqString change to 3.1, or at least after 3.0
has been branched from HEAD.

Reasoning: More work is required to polish it, and it doesn't really add
anything to the 3.0 release (beyond the bugfixes it triggered
indirectly), mainly preparation for future work.

Regards
Henrik

signature.asc (316 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SqString

Robert Collins
On Thu, 2007-05-24 at 11:19 +0200, Henrik Nordstrom wrote:

> Should we back out SqString for now until the implicit cast issues have
> been analyzed in more detail, or try fixing it somehow for the 3.0
> release?
>
> http://www.squid-cache.org/bugs/show_bug.cgi?id=1970
>
> My vote is to defer the SqString change to 3.1, or at least after 3.0
> has been branched from HEAD.
>
> Reasoning: More work is required to polish it, and it doesn't really add
> anything to the 3.0 release (beyond the bugfixes it triggered
> indirectly), mainly preparation for future work.
IIRC one of the reasons I didn't provide implicit casting initially was
because of these issues (and we were avoiding the STL at the time).

I'm in favour of backing it out.

Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

signature.asc (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SqString

Tsantilas Christos
In reply to this post by Henrik Nordström
Hi there,

Before SqString patch applied, squid3 looked stable, I had days to see a
problem.
The SqString had unpredicted effects in squid3 like the bug 1970. And
maybe there are other issues not appeared yet. It was not enough
analyzed before applied.

Regards,
     Christos

PS. Last weekend, my squid3 crashed again and again with segmentation
faults or assertions, in different places not allowing me to understand
where the problem is. OK, maybe happens because of bad compilation after
changes applied, but I am not absolutely sure....
However, I don't have such problems any more...


Henrik Nordstrom wrote:

> Should we back out SqString for now until the implicit cast issues have
> been analyzed in more detail, or try fixing it somehow for the 3.0
> release?
>
> http://www.squid-cache.org/bugs/show_bug.cgi?id=1970
>
> My vote is to defer the SqString change to 3.1, or at least after 3.0
> has been branched from HEAD.
>
> Reasoning: More work is required to polish it, and it doesn't really add
> anything to the 3.0 release (beyond the bugfixes it triggered
> indirectly), mainly preparation for future work.
>
> Regards
> Henrik
>  

Reply | Threaded
Open this post in threaded view
|

Re: SqString

Adrian Chadd
On Thu, May 24, 2007, Tsantilas Christos wrote:
> Hi there,
>
> Before SqString patch applied, squid3 looked stable, I had days to see a
> problem.
> The SqString had unpredicted effects in squid3 like the bug 1970. And
> maybe there are other issues not appeared yet. It was not enough
> analyzed before applied.

I'm leaning towards backing out SqString. We can always put it back
later after other tidyups are made.



Adrian

Reply | Threaded
Open this post in threaded view
|

Re: SqString

Alex Rousskov
In reply to this post by Henrik Nordström
On Thu, 2007-05-24 at 11:19 +0200, Henrik Nordstrom wrote:

> Should we back out SqString for now until the implicit cast issues have
> been analyzed in more detail, or try fixing it somehow for the 3.0
> release?
>
> http://www.squid-cache.org/bugs/show_bug.cgi?id=1970
>
> My vote is to defer the SqString change to 3.1, or at least after 3.0
> has been branched from HEAD.
>
> Reasoning: More work is required to polish it, and it doesn't really add
> anything to the 3.0 release (beyond the bugfixes it triggered
> indirectly), mainly preparation for future work.

I still think SqString "API" changes should not be in 3.0, but I do not
have a strong opinion and do not want to be the one backing them out.

We could adopt a middle-ground solution. The changes limited to renaming
String methods to match std::string API can stay. The changes
introducing new operators, conversions, or methods should be postponed
until v3.1. Same for changes in the code that uses Strings: renaming is
fine, rearranging and optimizing things is not.

HTH,

Alex.


Reply | Threaded
Open this post in threaded view
|

Re: SqString

Adrian Chadd
On Thu, May 24, 2007, Alex Rousskov wrote:

> I still think SqString "API" changes should not be in 3.0, but I do not
> have a strong opinion and do not want to be the one backing them out.
>
> We could adopt a middle-ground solution. The changes limited to renaming
> String methods to match std::string API can stay. The changes
> introducing new operators, conversions, or methods should be postponed
> until v3.1. Same for changes in the code that uses Strings: renaming is
> fine, rearranging and optimizing things is not.

Sounds good. At least squid-3 isn't crashing here under local light
testing now (with SqString). I'm going to load up squid-3 under some
heavy live reverse proxy load this weekend ; I'll report how it behaves.



Adrian

Reply | Threaded
Open this post in threaded view
|

Re: SqString

Amos Jeffries
Administrator
In reply to this post by Alex Rousskov
Alex Rousskov wrote:

> On Thu, 2007-05-24 at 11:19 +0200, Henrik Nordstrom wrote:
>> Should we back out SqString for now until the implicit cast issues have
>> been analyzed in more detail, or try fixing it somehow for the 3.0
>> release?
>>
>> http://www.squid-cache.org/bugs/show_bug.cgi?id=1970
>>
>> My vote is to defer the SqString change to 3.1, or at least after 3.0
>> has been branched from HEAD.
>>
>> Reasoning: More work is required to polish it, and it doesn't really add
>> anything to the 3.0 release (beyond the bugfixes it triggered
>> indirectly), mainly preparation for future work.
>
> I still think SqString "API" changes should not be in 3.0, but I do not
> have a strong opinion and do not want to be the one backing them out.
>
> We could adopt a middle-ground solution. The changes limited to renaming
> String methods to match std::string API can stay. The changes
> introducing new operators, conversions, or methods should be postponed
> until v3.1. Same for changes in the code that uses Strings: renaming is
> fine, rearranging and optimizing things is not.
>
> HTH,
>
> Alex.
>

I have just been experimenting with a few options short of a full backout.

My initial idea of dropping the constructor drags the changes into areas
the initial patch didn't touch. No go there.

I have had some success dropping the str* overloading entirely.
Replacing it all with a single casting operator which explicitly forces
the magic casts to reverse from the expensive copy 'up' toward string
into a cheap cast 'down' to a c_str() call.

Keep in mind this c_str() call being explicitly done all over squid at
present anyway. The only known risk is a buffer over- or under-run which
would occur anyway if the alternative explicit c_str() call was coded.

*please* test it this time. patch is attached or checkout and test the
squid3 branch labeled 'ayjwork'.

Alex: I'd particularly like a OK/DIES from you and your ICAP testing.


Amos

Patch file generated Fri May 25 20:18:12 NZST 2007 from
CVS branch ayjwork
CVS base branch HEAD
CVS repository: [hidden email]:/cvsroot/squid
CVS module: squid3

cvs -q rdiff -u -kk -r Z-ayjwork_merge_HEAD -r ayjwork squid3
Index: squid3/src/SqString.cc
diff -u squid3/src/SqString.cc:1.8 squid3/src/SqString.cc:1.1.2.16
--- squid3/src/SqString.cc:1.8 Mon May 21 18:51:11 2007
+++ squid3/src/SqString.cc Fri May 25 01:17:39 2007
@@ -240,16 +240,18 @@
 }
 
 const char&
-SqString::operator [](unsigned int pos) const
+SqString::operator [](int pos) const
 {
+    assert(pos >= 0 );
     assert(pos < size_ );
 
     return buf_[pos];
 }
 
 char&
-SqString::operator [](unsigned int pos)
+SqString::operator [](int pos)
 {
+    assert(pos >= 0 );
     assert(pos < size_ );
 
     return buf_[pos];
Index: squid3/src/SqString.h
diff -u squid3/src/SqString.h:1.4 squid3/src/SqString.h:1.1.2.7
--- squid3/src/SqString.h:1.4 Sun May 20 01:50:27 2007
+++ squid3/src/SqString.h Fri May 25 01:17:39 2007
@@ -87,6 +87,11 @@
     SqString (SqString const &);
     ~SqString();
 
+    //
+    // operator to cast to a const char *
+    //
+    operator char const *() const { return c_str(); }
+
     SqString &operator =(char const *);
     SqString &operator =(SqString const &);
     bool operator ==(SqString const &) const;
@@ -99,8 +104,8 @@
     _SQUID_INLINE_ int size() const;
     _SQUID_INLINE_ char const * c_str() const;
 
-    const char& operator [](unsigned int) const;
-    char& operator [](unsigned int);
+    const char& operator [](int) const;
+    char& operator [](int);
 
     void clear();
 
Index: squid3/src/SquidString.h
diff -u squid3/src/SquidString.h:1.9 squid3/src/SquidString.h:1.8.8.6
--- squid3/src/SquidString.h:1.9 Thu May 17 23:51:20 2007
+++ squid3/src/SquidString.h Fri May 25 01:17:39 2007
@@ -66,7 +66,6 @@
  *        initBuf(char*)  -> operator=(char*)
  *        reset(char*)    -> operator=(char*)
  *    - make init(char*) private for use by various assignment/costructor
- *    - define standard string operators
  *    - define debugs stream operator
  *
  */
@@ -83,18 +82,6 @@
 typedef SqString string;
 
 
-    /* Overload standard C functions using the basic string API */
-
-inline int strncasecmp(const string &lhs, const string &rhs, size_t len) { return strncasecmp(lhs.c_str(), rhs.c_str(), len); }
-inline int strcasecmp(const string &lhs, const string &rhs) { return strcasecmp(lhs.c_str(), rhs.c_str()); }
-
-inline int strncmp(const string &lhs, const string &rhs, size_t len) { return strncmp(lhs.c_str(), rhs.c_str(), len); }
-inline int strcmp(const string &lhs, const string &rhs) { return strcmp(lhs.c_str(), rhs.c_str()); }
-
-inline const char * strpbrk(const string &lhs, const string &rhs) { return strpbrk(lhs.c_str(), rhs.c_str()); }
-
-inline const char * strstr(const string &lhs, const string &rhs) { return strstr(lhs.c_str(), rhs.c_str()); }
-
 inline std::ostream& operator <<(std::ostream &os, const string &s) { os << s.c_str(); return os; }
 
 #endif /* SQUID_STRING_H */
Index: squid3/src/tests/testString.cc
diff -u squid3/src/tests/testString.cc:1.5 squid3/src/tests/testString.cc:1.2.12.4
--- squid3/src/tests/testString.cc:1.5 Wed May 23 14:51:22 2007
+++ squid3/src/tests/testString.cc Thu May 24 22:11:52 2007
@@ -82,7 +82,7 @@
 void
 testString::testAppend()
 {
-    // FIXME: make tests for this.
+    // FIXME: make more tests for this.
     string aStr("hello");
 
     aStr.append(" world");
Reply | Threaded
Open this post in threaded view
|

Re: SqString

Alex Rousskov
On Fri, 2007-05-25 at 20:39 +1200, Amos Jeffries wrote:

> I have just been experimenting with a few options short of a full backout.
>
> My initial idea of dropping the constructor drags the changes into areas
> the initial patch didn't touch. No go there.

Yes, of course.

> I have had some success dropping the str* overloading entirely.

Since the old code did not have those, I suspect the new code should
have "complete success" without them.

> Replacing it all with a single casting operator which explicitly forces
> the magic casts to reverse from the expensive copy 'up' toward string
> into a cheap cast 'down' to a c_str() call.

I am not sure why we need that operator now. On the IRC, you said that
it "it seems to be the only working alternative to complete removal of
the change" but I do not understand why.

Moreover, if we must have implicit casts for the new API to work,
perhaps we should back out the entire API change for now because we are
not good at predicting the side-effects of such implicit casts.

> Keep in mind this c_str() call being explicitly done all over squid at
> present anyway. The only known risk is a buffer over- or under-run which
> would occur anyway if the alternative explicit c_str() call was coded.

Here is my problem: The original API change changed some strcmp-like
calls. Some of the changes were incorrect. It is difficult for me to say
whether removing strcmp overloading and adding implicit char* casts will
expose all incorrect changes that we have not found yet. Unfortunately,
I doubt it.

What I would like to see is a diff against the pre-new-API code showing
that no calls got rewritten. Renaming a method is fine, but most other
changes should be postponed. The new patch you posted does not show me
how the main code is affected by the two changes combined, which is the
only thing that matters. Comparing with the current, broken code is not
very informative...

> *please* test it this time. patch is attached or checkout and test the
> squid3 branch labeled 'ayjwork'.
>
> Alex: I'd particularly like a OK/DIES from you and your ICAP testing.

We do not have a robust testing environment yet, so an "OK" from me
would not mean much.

Alex.


Reply | Threaded
Open this post in threaded view
|

Re: SqString

Amos Jeffries
Administrator
Alex Rousskov wrote:

> On Fri, 2007-05-25 at 20:39 +1200, Amos Jeffries wrote:
>
>> I have just been experimenting with a few options short of a full backout.
>>
>> My initial idea of dropping the constructor drags the changes into areas
>> the initial patch didn't touch. No go there.
>
> Yes, of course.
>
>> I have had some success dropping the str* overloading entirely.
>
> Since the old code did not have those, I suspect the new code should
> have "complete success" without them.
Ah, but it did. it just used a mix of some #define (2.x,3.0) and class
methods (3.0) and added upper case letters in the name.

>
>> Replacing it all with a single casting operator which explicitly forces
>> the magic casts to reverse from the expensive copy 'up' toward string
>> into a cheap cast 'down' to a c_str() call.
>
> I am not sure why we need that operator now. On the IRC, you said that
> it "it seems to be the only working alternative to complete removal of
> the change" but I do not understand why.

Ok let me make this clearer. the #defines and methods above did
_explicit_ calls to c_str().

At this stage we actually have a three-way choice:
  - backout
  - add in c_str() every place its needed.
  - add forced down-casting.

since the down-caster is simply an inline of c_str() the last two
options are the same, one with more work, one with less. Same actions.

With the downcasting its only 'implicit' in the nature of we cannot say
'its used there, and there, and there' without a full code audit.
Being part of string we can be certain that it will be be used only
where the old methods/defines were valid.


>
> Moreover, if we must have implicit casts for the new API to work,
> perhaps we should back out the entire API change for now because we are
> not good at predicting the side-effects of such implicit casts.
>

It's one of the options.

>> Keep in mind this c_str() call being explicitly done all over squid at
>> present anyway. The only known risk is a buffer over- or under-run which
>> would occur anyway if the alternative explicit c_str() call was coded.
>
> Here is my problem: The original API change changed some strcmp-like
> calls. Some of the changes were incorrect. It is difficult for me to say
> whether removing strcmp overloading and adding implicit char* casts will
> expose all incorrect changes that we have not found yet. Unfortunately,
> I doubt it.
>
> What I would like to see is a diff against the pre-new-API code showing
> that no calls got rewritten. Renaming a method is fine, but most other
> changes should be postponed. The new patch you posted does not show me
> how the main code is affected by the two changes combined, which is the
> only thing that matters. Comparing with the current, broken code is not
> very informative...
After you mentioned this on IRC I went back to my copy of the original
patch.
  Attached is a pseudo-patch (has stuff elided so I don't expect it will
map cleanly) showing all the str* changes and re-arranging  you are
worried about. All I have elided is the effected debug calls and
straight name mods; buf() to c_str() etc.
I have also left in the hash function-ptrs where I explicitly set the
namespace on strcmp so as not to clash their old values with anything new.


Amos

Patch file generated Fri May 18 18:34:42 NZST 2007 from
CVS branch ayjwork
CVS base branch HEAD
CVS repository: [hidden email]:/cvsroot/squid
CVS module: squid3

cvs -q rdiff -u -kk -r Z-ayjwork_merge_HEAD -r ayjwork squid3
Index: squid3/src/ACLHTTPHeaderData.cc
diff -u squid3/src/ACLHTTPHeaderData.cc:1.3 squid3/src/ACLHTTPHeaderData.cc:1.2.14.3
--- squid3/src/ACLHTTPHeaderData.cc:1.3 Sat Apr 28 15:51:47 2007
+++ squid3/src/ACLHTTPHeaderData.cc Wed May  2 05:53:05 2007
@@ -87,14 +87,14 @@
     char* t = strtokFile();
     assert (t != NULL);
     hdrName = t;
-    hdrId = httpHeaderIdByNameDef(hdrName.buf(), strlen(hdrName.buf()));
+    hdrId = httpHeaderIdByNameDef(hdrName.c_str(), hdrName.size());
     regex_rule->parse();
 }
 
Index: squid3/src/AuthUser.cc
diff -u squid3/src/AuthUser.cc:1.7 squid3/src/AuthUser.cc:1.3.4.3
--- squid3/src/AuthUser.cc:1.7 Wed May  9 01:51:04 2007
+++ squid3/src/AuthUser.cc Thu May 17 21:44:45 2007
@@ -143,7 +143,7 @@
     if (!proxy_auth_username_cache) {
         /* First time around, 7921 should be big enough */
         proxy_auth_username_cache =
-            hash_create((HASHCMP *) strcmp, 7921, hash_string);
+            hash_create((HASHCMP *) std::strcmp, 7921, hash_string);
         assert(proxy_auth_username_cache);
         eventAdd("User Cache Maintenance", cacheCleanup, NULL, Config.authenticateGCInterval, 1);
     }
Index: squid3/src/DelayTagged.cc
diff -u squid3/src/DelayTagged.cc:1.4 squid3/src/DelayTagged.cc:1.3.24.3
--- squid3/src/DelayTagged.cc:1.4 Sat Apr 28 15:51:48 2007
+++ squid3/src/DelayTagged.cc Mon May  7 03:32:21 2007
@@ -77,7 +77,7 @@
 DelayTaggedCmp(DelayTaggedBucket::Pointer const &left, DelayTaggedBucket::Pointer const &right)
 {
     /* for rate limiting, case insensitive */
-    return left->tag.caseCmp(right->tag.buf());
+    return strcasecmp(left->tag, right->tag);
 }
 
 void
Index: squid3/src/ESI.cc
diff -u squid3/src/ESI.cc:1.23 squid3/src/ESI.cc:1.22.8.3
--- squid3/src/ESI.cc:1.23 Sat Apr 28 15:51:48 2007
+++ squid3/src/ESI.cc Mon May  7 03:32:21 2007
@@ -2450,7 +2450,7 @@
              */
             return 0;
 
-        if (strstr (sctusable->content.buf(), "ESI/1.0"))
+        if (strstr (sctusable->content, "ESI/1.0"))
             rv = 1;
 
         httpHdrScTargetDestroy (sctusable);
Index: squid3/src/ESIVarState.cc
diff -u squid3/src/ESIVarState.cc:1.8 squid3/src/ESIVarState.cc:1.7.8.3
--- squid3/src/ESIVarState.cc:1.8 Sat Apr 28 15:51:48 2007
+++ squid3/src/ESIVarState.cc Mon May  7 03:32:22 2007
@@ -885,10 +885,9 @@
     if (!tempstr[0])
         return;
 
-    String strVary (rep->header.getList (HDR_VARY));
+    string strVary (rep->header.getList (HDR_VARY));
 
-    if (!strVary.size() || strVary.buf()[0] != '*') {
+    if (!strVary.size() || strVary[0] != '*') {
         rep->header.putStr (HDR_VARY, tempstr);
     }
 }
Index: squid3/src/HttpHdrRange.cc
diff -u squid3/src/HttpHdrRange.cc:1.15 squid3/src/HttpHdrRange.cc:1.13.8.3
--- squid3/src/HttpHdrRange.cc:1.15 Mon Apr 30 10:51:35 2007
+++ squid3/src/HttpHdrRange.cc Thu May  3 03:05:07 2007
@@ -253,14 +253,14 @@
     int count = 0;
     assert(this && range_spec);
     ++ParsedCount;
-    debugs(64, 8, "parsing range field: '" << range_spec->buf() << "'");
+    debugs(64, 8, "parsing range field: '" << *range_spec << "'");
     /* check range type */
 
-    if (range_spec->caseCmp("bytes=", 6))
+    if (strncasecmp(*range_spec,"bytes=", 6))
         return 0;
 
     /* skip "bytes="; hack! */
Index: squid3/src/HttpHdrSc.cc
diff -u squid3/src/HttpHdrSc.cc:1.6 squid3/src/HttpHdrSc.cc:1.5.8.5
--- squid3/src/HttpHdrSc.cc:1.6 Sat Apr 28 15:51:48 2007
+++ squid3/src/HttpHdrSc.cc Mon May  7 03:32:22 2007
@@ -370,9 +369,9 @@
     while (node) {
         HttpHdrScTarget *sct = (HttpHdrScTarget *)node->data;
 
-        if (target && sct->target.buf() && !strcmp (target, sct->target.buf()))
+        if (target && !sct->target.empty() && !strcmp(target, sct->target) )
             return sct;
-        else if (!target && !sct->target.buf())
+        else if (!target && sct->target.empty())
             return sct;
 
         node = node->next;
Index: squid3/src/HttpHeader.cc
diff -u squid3/src/HttpHeader.cc:1.44 squid3/src/HttpHeader.cc:1.40.4.7
--- squid3/src/HttpHeader.cc:1.44 Mon May  7 11:51:32 2007
+++ squid3/src/HttpHeader.cc Tue May  8 05:40:10 2007
@@ -602,7 +602,7 @@
             }
         }
 
-        if (e->id == HDR_OTHER && stringHasWhitespace(e->name.buf())) {
+        if (e->id == HDR_OTHER && strpbrk(e->name, w_space) != NULL) {
             debugs(55, Config.onoff.relaxed_header_parser <= 0 ? 1 : 2,
                    "WARNING: found whitespace in HTTP header name {" <<
                    getStringPrefix(field_start, field_end) << "}");
@@ -725,7 +725,7 @@
     debugs(55, 9, "deleting '" << name << "' fields in hdr " << this);
 
     while ((e = getEntry(&pos))) {
-        if (!e->name.caseCmp(name))
+        if (!strcasecmp(e->name,name))
             delAt(pos, count);
         else
             CBIT_SET(mask, e->id);
@@ -937,12 +937,12 @@
     if (id != -1)
         return getStrOrList(id);
 
-    String result;
+    string result;
 
     /* Sorry, an unknown header name. Do linear search */
     while ((e = getEntry(&pos))) {
-        if (e->id == HDR_OTHER && e->name.caseCmp(name) == 0) {
-            strListAdd(&result, e->value.buf(), ',');
+        if (e->id == HDR_OTHER && strcasecmp(e->name,name) == 0) {
+            strListAdd(&result, e->value.c_str(), ',');
         }
     }
 
@@ -1625,15 +1618,15 @@
 }
 
 http_hdr_type
-httpHeaderIdByName(const char *name, int name_len, const HttpHeaderFieldInfo * info, int end)
+httpHeaderIdByName(const char *name, unsigned int name_len, const HttpHeaderFieldInfo * info, int end)
 {
     int i;
 
     for (i = 0; i < end; ++i) {
-        if (name_len >= 0 && name_len != info[i].name.size())
+        if (name_len >= 0 && name_len != (unsigned int)info[i].name.size())
             continue;
 
-        if (!strncasecmp(name, info[i].name.buf(),
+        if (!strncasecmp(name, info[i].name,
                          name_len < 0 ? info[i].name.size() + 1 : name_len))
             return info[i].id;
     }
Index: squid3/src/HttpReply.cc
diff -u squid3/src/HttpReply.cc:1.39 squid3/src/HttpReply.cc:1.38.4.5
--- squid3/src/HttpReply.cc:1.39 Fri Apr 20 00:58:35 2007
+++ squid3/src/HttpReply.cc Mon May  7 03:32:22 2007
@@ -278,9 +278,7 @@
 
     two = otherRep->header.getStrOrList(HDR_ETAG);
 
-    if (!one.buf() || !two.buf() || strcasecmp (one.buf(), two.buf())) {
-        one.clean();
-        two.clean();
+    if (one.empty() || two.empty() || strcasecmp (one, two)) {
         return 0;
     }
 
@@ -292,9 +290,9 @@
 
     two = otherRep->header.getStrOrList(HDR_CONTENT_MD5);
 
-    if (!one.buf() || !two.buf() || strcasecmp (one.buf(), two.buf())) {
-        one.clean();
-        two.clean();
+    if (one.empty() || two.empty() || strcasecmp (one, two)) {
+        one.clear();
+        two.clear();
         return 0;
     }
 
@@ -435,8 +433,8 @@
 
 bool HttpReply::sanityCheckStartLine(MemBuf *buf, http_status *error)
 {
-    if (buf->contentSize() >= protoPrefix.size() && protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) {
-        debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol prefix (" << protoPrefix.buf() << ") in '" << buf->content() << "'");
+    if (buf->contentSize() >= protoPrefix.size() && strncmp(protoPrefix, buf->content(), protoPrefix.size()) != 0) {
+        debugs(58, 3, "HttpReply::sanityCheckStartLine: missing protocol prefix (" << protoPrefix << ") in '" << buf->content() << "'");
         *error = HTTP_INVALID_HEADER;
         return false;
     }
Index: squid3/src/HttpStatusLine.cc
diff -u squid3/src/HttpStatusLine.cc:1.7 squid3/src/HttpStatusLine.cc:1.5.12.4
--- squid3/src/HttpStatusLine.cc:1.7 Fri May  4 15:51:11 2007
+++ squid3/src/HttpStatusLine.cc Sat May  5 07:37:31 2007
@@ -90,7 +90,7 @@
     // XXX: HttpMsg::parse() has a similar check but is using
     // casesensitive comparison (which is required by HTTP errata?)
 
-    if (protoPrefix.caseCmp(start, protoPrefix.size()) != 0)
+    if (strncasecmp(protoPrefix,start, protoPrefix.size()) != 0)
         return 0;
 
     start += protoPrefix.size();
Index: squid3/src/access_log.cc
diff -u squid3/src/access_log.cc:1.45 squid3/src/access_log.cc:1.41.4.8
--- squid3/src/access_log.cc:1.45 Thu May 17 13:51:55 2007
+++ squid3/src/access_log.cc Thu May 17 22:24:38 2007
@@ -1631,8 +1631,8 @@
 static void
 fvdbInit(void)
 {
-    via_table = hash_create((HASHCMP *) strcmp, 977, hash4);
-    forw_table = hash_create((HASHCMP *) strcmp, 977, hash4);
+    via_table = hash_create((HASHCMP *) std::strcmp, 977, hash4);
+    forw_table = hash_create((HASHCMP *) std::strcmp, 977, hash4);
 }
 
 static void
@@ -1717,10 +1717,10 @@
 {
     hashFreeItems(via_table, fvdbFreeEntry);
     hashFreeMemory(via_table);
-    via_table = hash_create((HASHCMP *) strcmp, 977, hash4);
+    via_table = hash_create((HASHCMP *) std::strcmp, 977, hash4);
     hashFreeItems(forw_table, fvdbFreeEntry);
     hashFreeMemory(forw_table);
-    forw_table = hash_create((HASHCMP *) strcmp, 977, hash4);
+    forw_table = hash_create((HASHCMP *) std::strcmp, 977, hash4);
 }
 
 #endif
Index: squid3/src/client_db.cc
diff -u squid3/src/client_db.cc:1.11 squid3/src/client_db.cc:1.10.8.2
--- squid3/src/client_db.cc:1.11 Sat Apr 28 15:51:49 2007
+++ squid3/src/client_db.cc Thu May  3 06:37:05 2007
@@ -80,7 +80,7 @@
     if (client_table)
         return;
 
-    client_table = hash_create((HASHCMP *) strcmp, CLIENT_DB_HASH_SIZE, hash_string);
+    client_table = hash_create((HASHCMP *) std::strcmp, CLIENT_DB_HASH_SIZE, hash_string);
 }
 
 void
Index: squid3/src/dns_internal.cc
diff -u squid3/src/dns_internal.cc:1.40 squid3/src/dns_internal.cc:1.38.2.2
--- squid3/src/dns_internal.cc:1.40 Mon Apr 30 10:51:35 2007
+++ squid3/src/dns_internal.cc Thu May  3 06:37:05 2007
@@ -1238,7 +1238,7 @@
     if (!init) {
         memDataInit(MEM_IDNS_QUERY, "idns_query", sizeof(idns_query), 0);
         memset(RcodeMatrix, '\0', sizeof(RcodeMatrix));
-        idns_lookup_hash = hash_create((HASHCMP *) strcmp, 103, hash_string);
+        idns_lookup_hash = hash_create((HASHCMP *) std::strcmp, 103, hash_string);
         init++;
     }
 }
Index: squid3/src/external_acl.cc
diff -u squid3/src/external_acl.cc:1.54 squid3/src/external_acl.cc:1.52.4.5
--- squid3/src/external_acl.cc:1.54 Sat Apr 28 15:51:52 2007
+++ squid3/src/external_acl.cc Thu May  3 06:37:05 2007
@@ -1270,7 +1270,7 @@
 
     for (p = Config.externalAclHelperList; p; p = p->next) {
         if (!p->cache)
-            p->cache = hash_create((HASHCMP *) strcmp, hashPrime(1024), hash4);
+            p->cache = hash_create((HASHCMP *) std::strcmp, hashPrime(1024), hash4);
 
         if (!p->theHelper)
             p->theHelper = helperCreate(p->name);
Index: squid3/src/fqdncache.cc
diff -u squid3/src/fqdncache.cc:1.19 squid3/src/fqdncache.cc:1.18.8.2
--- squid3/src/fqdncache.cc:1.19 Sat Apr 28 15:51:52 2007
+++ squid3/src/fqdncache.cc Thu May  3 06:37:05 2007
@@ -527,7 +527,7 @@
 
     n = hashPrime(fqdncache_high / 4);
 
-    fqdn_table = hash_create((HASHCMP *) strcmp, n, hash4);
+    fqdn_table = hash_create((HASHCMP *) std::strcmp, n, hash4);
 
     memDataInit(MEM_FQDNCACHE_ENTRY, "fqdncache_entry",
                 sizeof(fqdncache_entry), 0);
Index: squid3/src/ftp.cc
diff -u squid3/src/ftp.cc:1.73 squid3/src/ftp.cc:1.62.4.9
--- squid3/src/ftp.cc:1.73 Mon May  7 14:51:23 2007
+++ squid3/src/ftp.cc Tue May  8 05:40:11 2007
@@ -2959,7 +2959,7 @@
     if (!ftpState->flags.isdir && /* Not a directory */
             !ftpState->flags.try_slash_hack && /* Not in slash hack */
             ftpState->mdtm <= 0 && ftpState->size < 0 && /* Not known as a file */
-            ftpState->request->urlpath.caseCmp("/%2f", 4) != 0) { /* No slash encoded */
+            strncasecmp(ftpState->request->urlpath, "/%2f", 4) != 0) { /* No slash encoded */
 
         switch (ftpState->state) {
 
Index: squid3/src/ident.cc
diff -u squid3/src/ident.cc:1.16 squid3/src/ident.cc:1.15.4.2
--- squid3/src/ident.cc:1.16 Sat Apr 28 15:51:53 2007
+++ squid3/src/ident.cc Thu May  3 06:37:06 2007
@@ -270,7 +270,7 @@
 void
 identInit(void)
 {
-    ident_hash = hash_create((HASHCMP *) strcmp,
+    ident_hash = hash_create((HASHCMP *) std::strcmp,
                              hashPrime(Squid_MaxFD / 8),
                              hash4);
 }
Index: squid3/src/ipcache.cc
diff -u squid3/src/ipcache.cc:1.20 squid3/src/ipcache.cc:1.19.8.2
--- squid3/src/ipcache.cc:1.20 Sat Apr 28 15:51:53 2007
+++ squid3/src/ipcache.cc Thu May  3 06:37:06 2007
@@ -586,7 +586,7 @@
     ipcache_low = (long) (((float) Config.ipcache.size *
                            (float) Config.ipcache.low) / (float) 100);
     n = hashPrime(ipcache_high / 4);
-    ip_table = hash_create((HASHCMP *) strcmp, n, hash4);
+    ip_table = hash_create((HASHCMP *) std::strcmp, n, hash4);
     memDataInit(MEM_IPCACHE_ENTRY, "ipcache_entry", sizeof(ipcache_entry), 0);
 }
 
Index: squid3/src/net_db.cc
diff -u squid3/src/net_db.cc:1.30 squid3/src/net_db.cc:1.27.8.3
--- squid3/src/net_db.cc:1.30 Mon Apr 30 10:51:40 2007
+++ squid3/src/net_db.cc Tue May  8 23:59:36 2007
@@ -892,11 +892,11 @@
 
     n = hashPrime(Config.Netdb.high / 4);
 
-    addr_table = hash_create((HASHCMP *) strcmp, n, hash_string);
+    addr_table = hash_create((HASHCMP *) std::strcmp, n, hash_string);
 
     n = hashPrime(3 * Config.Netdb.high / 4);
 
-    host_table = hash_create((HASHCMP *) strcmp, n, hash_string);
+    host_table = hash_create((HASHCMP *) std::strcmp, n, hash_string);
 
     eventAddIsh("netdbSaveState", netdbSaveState, NULL, 3600.0, 1);
 
Index: squid3/src/pconn.cc
diff -u squid3/src/pconn.cc:1.17 squid3/src/pconn.cc:1.14.2.3
--- squid3/src/pconn.cc:1.17 Fri May 11 06:51:19 2007
+++ squid3/src/pconn.cc Thu May 17 21:44:46 2007
@@ -217,7 +217,7 @@
 PconnPool::PconnPool(const char *aDescr) : table(NULL), descr(aDescr)
 {
     int i;
-    table = hash_create((HASHCMP *) strcmp, 229, hash_string);
+    table = hash_create((HASHCMP *) std::strcmp, 229, hash_string);
 
     for (i = 0; i < PCONN_HIST_SZ; i++)
         hist[i] = 0;
Index: squid3/src/url.cc
diff -u squid3/src/url.cc:1.18 squid3/src/url.cc:1.17.4.3
--- squid3/src/url.cc:1.18 Sat Apr 28 15:51:56 2007
+++ squid3/src/url.cc Tue May  1 17:09:51 2007
@@ -268,7 +268,7 @@
     for (t = host; *t; t++)
         *t = xtolower(*t);
 
-    if (stringHasWhitespace(host)) {
+    if (strpbrk(host, w_space) != NULL) {
         if (URI_WHITESPACE_STRIP == Config.uri_whitespace) {
             t = q = host;
 
@@ -316,7 +316,7 @@
     }
 
 #endif
-    if (stringHasWhitespace(urlpath)) {
+    if (strpbrk(urlpath, w_space) != NULL) {
         debugs(23, 2, "urlParse: URI has whitespace: {" << url << "}");
 
         switch (Config.uri_whitespace) {
@@ -410,6 +410,22 @@
     return (request->canonical = xstrdup(urlbuf));
 }
 
+int
+stringHasCntl(const char *s)
+{
+    unsigned char c;
+
+    while ((c = (unsigned char) *s++) != '\0') {
+        if (c <= 0x1f)
+            return 1;
+
+        if (c >= 0x7f && c <= 0x9f)
+            return 1;
+    }
+
+    return 0;
+}
+
 char *
 urlCanonicalClean(const HttpRequest * request)
 {
Index: squid3/src/urn.cc
diff -u squid3/src/urn.cc:1.27 squid3/src/urn.cc:1.25.8.4
--- squid3/src/urn.cc:1.27 Sat Apr 28 15:51:56 2007
+++ squid3/src/urn.cc Thu May  3 03:05:14 2007
@@ -193,7 +193,7 @@
 bool
 UrnState::RequestNeedsMenu(HttpRequest *r)
 {
-    return strncasecmp(r->urlpath.buf(), "menu.", 5) == 0;
+    return strncasecmp(r->urlpath, "menu.", 5) == 0;
 }
 
 void
Index: squid3/src/ICAP/ICAPOptions.cc
diff -u squid3/src/ICAP/ICAPOptions.cc:1.9 squid3/src/ICAP/ICAPOptions.cc:1.9.4.2
--- squid3/src/ICAP/ICAPOptions.cc:1.9 Thu Apr  5 22:52:43 2007
+++ squid3/src/ICAP/ICAPOptions.cc Mon May  7 03:32:23 2007
@@ -172,8 +172,8 @@
         if (eLen < urlLen) {
             const int eOff = urlLen - eLen;
             // RFC 3507 examples imply that extensions come without leading '.'
-            if (urlPath.buf()[eOff-1] == '.' &&
-                strcmp(urlPath.buf() + eOff, e->key) == 0) {
+            if (urlPath[eOff-1] == '.' &&
+                strcmp(&urlPath[eOff], e->key) == 0) {
                 debugs(93,7, "ICAPOptions url " << urlPath << " matches " <<
                     name << " extension " << e->key);
                 return true;
Index: squid3/src/ICAP/ICAPServiceRep.cc
diff -u squid3/src/ICAP/ICAPServiceRep.cc:1.10 squid3/src/ICAP/ICAPServiceRep.cc:1.7.4.6
--- squid3/src/ICAP/ICAPServiceRep.cc:1.10 Tue May  8 09:51:28 2007
+++ squid3/src/ICAP/ICAPServiceRep.cc Tue May  8 23:59:37 2007
@@ -80,24 +80,24 @@
 
     char *service_type = NULL;
 
-    ConfigParser::ParseString(&key);
+    ConfigParser::ParseString(key);
     ConfigParser::ParseString(&service_type);
     ConfigParser::ParseBool(&bypass);
-    ConfigParser::ParseString(&uri);
+    ConfigParser::ParseString(uri);
 
-    debugs(3, 5, "ICAPService::parseConfigLine (line " << config_lineno << "): " << key.buf() << " " << service_type << " " << bypass);
+    debugs(3, 5, "ICAPService::parseConfigLine (line " << config_lineno << "): " << key << " " << service_type << " " << bypass);
 
     method = parseMethod(service_type);
     point = parseVectPoint(service_type);
 
     debugs(3, 5, "ICAPService::parseConfigLine (line " << config_lineno << "): service is " << methodStr() << "_" << vectPointStr());
 
-    if (uri.cmp("icap://", 7) != 0) {
-        debugs(3, 0, "ICAPService::parseConfigLine (line " << config_lineno << "): wrong uri: " << uri.buf());
+    if (strncmp(uri, "icap://", 7) != 0) {
+        debugs(3, 0, "ICAPService::parseConfigLine (line " << config_lineno << "): wrong uri: " << uri);
         return false;
     }
 
-    const char *s = uri.buf() + 7;
+    const char *s = &uri[7];
 
     const char *e;
 
Index: squid3/src/auth/digest/auth_digest.cc
diff -u squid3/src/auth/digest/auth_digest.cc:1.34 squid3/src/auth/digest/auth_digest.cc:1.29.4.6
--- squid3/src/auth/digest/auth_digest.cc:1.34 Wed May  9 02:51:14 2007
+++ squid3/src/auth/digest/auth_digest.cc Thu May 17 21:44:48 2007
@@ -204,7 +204,7 @@
         digest_nonce_pool = memPoolCreate("Digest Scheme nonce's", sizeof(digest_nonce_h));
 
     if (!digest_nonce_cache) {
-        digest_nonce_cache = hash_create((HASHCMP *) strcmp, 7921, hash_string);
+        digest_nonce_cache = hash_create((HASHCMP *) std::strcmp, 7921, hash_string);
         assert(digest_nonce_cache);
         eventAdd("Digest none cache maintenance", authenticateDigestNonceCacheCleanup, NULL, digestConfig.nonceGCInterval, 1);
     }
Index: squid3/src/auth/negotiate/auth_negotiate.cc
diff -u squid3/src/auth/negotiate/auth_negotiate.cc:1.16 squid3/src/auth/negotiate/auth_negotiate.cc:1.11.4.4
--- squid3/src/auth/negotiate/auth_negotiate.cc:1.16 Wed May  9 02:51:14 2007
+++ squid3/src/auth/negotiate/auth_negotiate.cc Thu May 17 21:44:48 2007
@@ -180,7 +180,7 @@
             negotiateauthenticators = helperStatefulCreate("negotiateauthenticator");
 
         if (!proxy_auth_cache)
-            proxy_auth_cache = hash_create((HASHCMP *) strcmp, 7921, hash_string);
+            proxy_auth_cache = hash_create((HASHCMP *) std::strcmp, 7921, hash_string);
 
         assert(proxy_auth_cache);
 
Index: squid3/src/auth/ntlm/auth_ntlm.cc
diff -u squid3/src/auth/ntlm/auth_ntlm.cc:1.38 squid3/src/auth/ntlm/auth_ntlm.cc:1.33.4.4
--- squid3/src/auth/ntlm/auth_ntlm.cc:1.38 Wed May  9 02:51:14 2007
+++ squid3/src/auth/ntlm/auth_ntlm.cc Thu May 17 21:44:48 2007
@@ -181,7 +181,7 @@
             ntlmauthenticators = helperStatefulCreate("ntlmauthenticator");
 
         if (!proxy_auth_cache)
-            proxy_auth_cache = hash_create((HASHCMP *) strcmp, 7921, hash_string);
+            proxy_auth_cache = hash_create((HASHCMP *) std::strcmp, 7921, hash_string);
 
         assert(proxy_auth_cache);
 
Reply | Threaded
Open this post in threaded view
|

Re: SqString

Adrian Chadd
Is there any reason for:

-    if (stringHasWhitespace(host)) {
+    if (strpbrk(host, w_space) != NULL) {


Can't you fix stringHasWhitespace, which is more explicit?



Adrian
Reply | Threaded
Open this post in threaded view
|

Re: SqString

Alex Rousskov
In reply to this post by Amos Jeffries
On Sat, 2007-05-26 at 04:08 +1200, Amos Jeffries wrote:

> Alex Rousskov wrote:
> > On Fri, 2007-05-25 at 20:39 +1200, Amos Jeffries wrote:
> >
> >> I have just been experimenting with a few options short of a full backout.
> >>
> >> My initial idea of dropping the constructor drags the changes into areas
> >> the initial patch didn't touch. No go there.
> >
> > Yes, of course.
> >
> >> I have had some success dropping the str* overloading entirely.
> >
> > Since the old code did not have those, I suspect the new code should
> > have "complete success" without them.
>
> Ah, but it did. it just used a mix of some #define (2.x,3.0) and class
> methods (3.0) and added upper case letters in the name.

Class methods (versus overloaded globals) make a big difference when it
comes to silent type conversion (or lack of it). That is, in part, the
reason why the replacements in the new API where not functionally
equivalent to the original code.

> Ok let me make this clearer. the #defines and methods above did
> _explicit_ calls to c_str().

And you want to remove those methods because ...? Can you just rename
them to match the STL naming scheme? Does the problem boil down to not
having an STL equivalent of some custom String methods?

> > What I would like to see is a diff against the pre-new-API code showing
> > that no calls got rewritten. Renaming a method is fine, but most other
> > changes should be postponed. The new patch you posted does not show me
> > how the main code is affected by the two changes combined, which is the
> > only thing that matters. Comparing with the current, broken code is not
> > very informative...
>
> After you mentioned this on IRC I went back to my copy of the original
> patch.
>   Attached is a pseudo-patch (has stuff elided so I don't expect it will
> map cleanly) showing all the str* changes and re-arranging  you are
> worried about. All I have elided is the effected debug calls and
> straight name mods; buf() to c_str() etc.
> I have also left in the hash function-ptrs where I explicitly set the
> namespace on strcmp so as not to clash their old values with anything new.

I am uncomfortable with the changes in the latest patch because they
change the working code. I did not do full analysis, but here are a few
specific places that worry me:

1) String::buf() did not return NULL for an empty but initialized
String. SqString::empty() does:

> -        if (target && sct->target.buf() && !strcmp (target,
> +        if (target && !sct->target.empty() && !strcmp(target,

and

> -        else if (!target && !sct->target.buf())
> +        else if (!target && sct->target.empty())

and possibly elsewhere.


2) String::cmp does not just call strncmp so I cannot be sure this and
other similar changes are simple renaming:

> -    if (buf->contentSize() >= protoPrefix.size() &&
> protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) {
> +    if (buf->contentSize() >= protoPrefix.size() &&
> strncmp(protoPrefix, buf->content(), protoPrefix.size()) != 0) {

3) I am not sure C++ guarantees the following to be what we want it to
be so this might cause problems in the future:

> -                strcmp(urlPath.buf() + eOff, e->key) == 0) {
> +                strcmp(&urlPath[eOff], e->key) == 0) {

or

> -    const char *s = uri.buf() + 7;
> +    const char *s = &uri[7];


Are all of the above and other changes correct? I do not know. It is not
obvious to me that they are. If you are confident, and others do not
object, commit them (no need to prove the correctness to me!). If you
are not confident, let's back the whole thing out for now and then fix a
couple of problems that you have found while working on this. Once 3.0
is branched, let's discuss whether and how to switch to STL strings.

Thank you,

Alex.


Reply | Threaded
Open this post in threaded view
|

Re: SqString

Amos Jeffries
Administrator
Alex Rousskov wrote:

> On Sat, 2007-05-26 at 04:08 +1200, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>> On Fri, 2007-05-25 at 20:39 +1200, Amos Jeffries wrote:
>>>
>>>> I have just been experimenting with a few options short of a full backout.
>>>>
>>>> My initial idea of dropping the constructor drags the changes into areas
>>>> the initial patch didn't touch. No go there.
>>> Yes, of course.
>>>
>>>> I have had some success dropping the str* overloading entirely.
>>> Since the old code did not have those, I suspect the new code should
>>> have "complete success" without them.
>> Ah, but it did. it just used a mix of some #define (2.x,3.0) and class
>> methods (3.0) and added upper case letters in the name.
>
> Class methods (versus overloaded globals) make a big difference when it
> comes to silent type conversion (or lack of it). That is, in part, the
> reason why the replacements in the new API where not functionally
> equivalent to the original code.

True, But please understand, my reason for picking the overload path in
the first place was the amount of times that path was taken in the
original code.

>
>> Ok let me make this clearer. the #defines and methods above did
>> _explicit_ calls to c_str().
>
> And you want to remove those methods because ...? Can you just rename
> them to match the STL naming scheme?

See for yourself:

  int
  String::caseCmp (char const *aString) const
  {
      return strcasecmp(buf(), aString);
  }

  int
  String::caseCmp (char const *aString, size_t count) const
  {
      return strncasecmp(buf(), aString, count);
  }

Now spot the benefit in that duplicated code...
Secondly, they were a custom API to the old String:: that was used in
only a few places with the non-wrapped version still used over most of
the code.

And how is the above different from:

  strncasecmp(str.buf(), aStr, count);

OR

  strncasecmp((char*)str, aStr, count);
   [ with  (char*)str ==> str.buf() ].


 > Does the problem boil down to not
> having an STL equivalent of some custom String methods?

No as the above shows they are _excatly_ equivalent.

All the problems I have seen are due to the magic of C++ choosing the
expensive method of conversion. If there are other bugs or posts than
1967 and 1970 that might effect my view on this please point them out.

>
>>> What I would like to see is a diff against the pre-new-API code showing
>>> that no calls got rewritten. Renaming a method is fine, but most other
>>> changes should be postponed. The new patch you posted does not show me
>>> how the main code is affected by the two changes combined, which is the
>>> only thing that matters. Comparing with the current, broken code is not
>>> very informative...
 >>

>> After you mentioned this on IRC I went back to my copy of the original
>> patch.
>>   Attached is a pseudo-patch (has stuff elided so I don't expect it will
>> map cleanly) showing all the str* changes and re-arranging  you are
>> worried about. All I have elided is the effected debug calls and
>> straight name mods; buf() to c_str() etc.
>> I have also left in the hash function-ptrs where I explicitly set the
>> namespace on strcmp so as not to clash their old values with anything new.
>
> I am uncomfortable with the changes in the latest patch because they
> change the working code. I did not do full analysis, but here are a few
> specific places that worry me:
>
> 1) String::buf() did not return NULL for an empty but initialized
> String. SqString::empty() does:

WTF? here is the original code:
char const *
String::buf() const
{
      return buf_;
}

WHERE: buf_ is either NULL, or a char*. NO initialisation at all going
on there.

WHERE empty() returns (size()==0 || buf_ == NULL);
   TRUE - IFF buf_ == char*
   FALSE - IFF buf_ ==

I the case(s) you point out below the initial operation of buf()
provided a buffer-overrun in the edge case where  buf_ != NULL, it
attempted to use it in a comparison or function call. **Even if it was
full of non-null garbage and the len_ was officially 0.***

>
>> -        if (target && sct->target.buf() && !strcmp (target,
>> +        if (target && !sct->target.empty() && !strcmp(target,
>
> and
>
>> -        else if (!target && !sct->target.buf())
>> +        else if (!target && sct->target.empty())
>
> and possibly elsewhere.
>
>
> 2) String::cmp does not just call strncmp so I cannot be sure this and
> other similar changes are simple renaming:
>


No, cmp was mapped to compare, which performs the internal operation of
strncmp (doing its own internal check on size()).

Consider the two following code paths:

(cmp / compare)
   - test len_ == 0                    - return 0 == param[0]
   - test this* for being null/empty   - return 0 == param[0]
   - test para for being null or empty - return 0 == buf_[0]
   - retrn output of strncmp(buf_, param, len_)

(strncmp)
   - test strlen(lhs) == 0             - return 0 == rhs[0]
   - test strlen(rhs) == 0             - return 0 == lhs[0]
   - skip down for strlen places
     OR rhs[p] != lhs[p]
   - return lhs[p] - rhs[p];

So you can see the cmp(str, len) calls were simply performing the same
pre-check operations of strncmp twice for any non-null string.
size() also performs those checks and does so on behalf of the unwrapped
strcmp()'s

The 'simple renaming' as you call it were either the bits I got to
before doing the unit tests that showed me this, or bits where I could
not be sure size() was available beforehand to perform the limiting on
strncmp explicitly.

I wrote a full set of unit-tests to verify all these cases and check the
behaviour output was as expected before doing the API upgrade.


>> -    if (buf->contentSize() >= protoPrefix.size() &&
>> protoPrefix.cmp(buf->content(), protoPrefix.size()) != 0) {
>> +    if (buf->contentSize() >= protoPrefix.size() &&
>> strncmp(protoPrefix, buf->content(), protoPrefix.size()) != 0) {
>
> 3) I am not sure C++ guarantees the following to be what we want it to
> be so this might cause problems in the future:
>
>> -                strcmp(urlPath.buf() + eOff, e->key) == 0) {
>> +                strcmp(&urlPath[eOff], e->key) == 0) {
>
> or
>
>> -    const char *s = uri.buf() + 7;
>> +    const char *s = &uri[7];
>
>
> Are all of the above and other changes correct? I do not know.

I am, so long as your who wrote that code are using the int type given
to eOff properly (as I see you doing in that code) as a whole-number
offset in bytes from the start of buf_.

I am simply using [] to wrap the offset ptr-addition and in a single
place add the buffer-overrun safety-check that has been long missing. I
have used the proper semantics of [n] as meaning n-byte offset on a char
array.

Use of buf_ like that original above code was part of the two _known_
causes of buffer-violation in squid that I found commented in the
original String files.


 > It is not
> obvious to me that they are. If you are confident, and others do not
> object, commit them (no need to prove the correctness to me!).

I think we have a talking error here. What did you think this was?
These were only the parts of the 'broken' code that were changed and
would likely remain changed after the latest update. NOTHING outside
SqString.* would change to fix all the broke.

I am confident, was before I submit anything to HEAD. Doesn't mean I'm
perfectly right. What I would like is to be sure that you understand
these changes. In light of my reasoning for a the above If you can still
provide a valid case of any of them being bad I'll agree that bad code
should not be added.

What my latest update fix would do it is terminate the 'bad magic' with
prejudice and a suitable equivalent.


>  If you
> are not confident, let's back the whole thing out for now and then fix a
> couple of problems that you have found while working on this. Once 3.0
> is branched, let's discuss whether and how to switch to STL strings.
>
> Thank you,
>
> Alex.
>


Reply | Threaded
Open this post in threaded view
|

Re: SqString

Alex Rousskov

-- Please note that I am only [briefly] responding in hope to improve
future SqString patches. I do realize that this is no longer relevant to
Squid 3.0.

On Sat, 2007-05-26 at 18:41 +1200, Amos Jeffries wrote:

> Alex Rousskov wrote:
> >
> > And you want to remove those methods because ...? Can you just rename
> > them to match the STL naming scheme?
>
> See for yourself:
>
>   int
>   String::caseCmp (char const *aString) const
>   {
>       return strcasecmp(buf(), aString);
>   }
>
>   int
>   String::caseCmp (char const *aString, size_t count) const
>   {
>       return strncasecmp(buf(), aString, count);
>   }
>
> Now spot the benefit in that duplicated code...

I do not see code duplication and can name a couple of benefits. Please
see below for some of them.

> Secondly, they were a custom API to the old String:: that was used in
> only a few places with the non-wrapped version still used over most of
> the code.

True, but hardly an API's fault. If we want to stick with a custom
String long-term, the "most of the code" can be changed to use the right
API.

> And how is the above different from:
>
>   strncasecmp(str.buf(), aStr, count);
>
> OR
>
>   strncasecmp((char*)str, aStr, count);
>    [ with  (char*)str ==> str.buf() ].
>

There are a few differences: The calls with the old API are shorter and
less confusing (e.g., do I cast the first parameter to char*, use
str.buf(), or just str with the new API?). I can also track and modify
the original code in one place, without searching for and modifying all
strncasecmp calls.

>  > Does the problem boil down to not
> > having an STL equivalent of some custom String methods?
>
> No as the above shows they are _excatly_ equivalent.

Obviously, we disagree.

> > 1) String::buf() did not return NULL for an empty but initialized
> > String. SqString::empty() does:
>
> WTF? here is the original code:
> char const *
> String::buf() const
> {
>       return buf_;
> }
>
> WHERE: buf_ is either NULL, or a char*. NO initialisation at all going
> on there.
>
> WHERE empty() returns (size()==0 || buf_ == NULL);
>    TRUE - IFF buf_ == char*
>    FALSE - IFF buf_ ==

I am saying that !String::buf() might return "false" when
String::empty() returns true. When you substitute one for the other, I
begin to worry about new bugs. Consider an empty string (""), for
example. Does it have zero size? Is its buf() NULL?

What may look like trivial polishing may result in hard-to-find bugs. We
should not mix the two steps (zero-effect core API change and
polishing), if possible.

> >
> >> -        if (target && sct->target.buf() && !strcmp (target,
> >> +        if (target && !sct->target.empty() && !strcmp(target,
> >
> > and
> >
> >> -        else if (!target && !sct->target.buf())
> >> +        else if (!target && sct->target.empty())
> >
> > and possibly elsewhere.

> >
> > 3) I am not sure C++ guarantees the following to be what we want it to
> > be so this might cause problems in the future:
> >
> >> -                strcmp(urlPath.buf() + eOff, e->key) == 0) {
> >> +                strcmp(&urlPath[eOff], e->key) == 0) {
> >
> > or
> >
> >> -    const char *s = uri.buf() + 7;
> >> +    const char *s = &uri[7];

> I am, so long as your who wrote that code are using the int type given
> to eOff properly (as I see you doing in that code) as a whole-number
> offset in bytes from the start of buf_.

I was talking about a standard C++ std::string here (the eventual
zero-code-change replacement for SqString, right?) not our custom String
code. I do not know how "address of a character" operation is defined
for STL strings and whether there are any performance implications to
worry about.

HTH,

Alex.