Re: [PATCH] SBuf conversion of HttpRequestMethod

From: Amos Jeffries <squid3_at_treenet.co.nz>
Date: Fri, 11 Apr 2014 17:32:53 +1200

On 11/04/2014 10:24 a.m., Alex Rousskov wrote:
> On 04/10/2014 05:47 AM, Amos Jeffries wrote:
>> New patch attached.
>> This clears up the requests from both of you.
>
> Not all of them, and there is also a new serious bug. Details below.
>

Okay. So the critical issue(s) are in the half-baked SBuf API extension.
Posting that as a separate patch and blocking this patch until that is
complete.

I believe the attached patch is complete regarding the method conversion.

>
> Old stuff:
>
> * The usually needless SBuf allocation for common methods is still there:
>
>> + SBuf str(begin, end-begin);
>> +
>> + // TODO: Optimize this linear search.
>> for (++theMethod; theMethod < Http::METHOD_ENUM_END; ++theMethod) {
> ...
>
> It should be avoided, and I thought adding SBuf::cmp(c-string) methods
> was done specifically to avoid it. I do not even see those new methods
> used in this code.
>

Not the loop. Just to avoid the need for "SBuf str(begin, end-begin);"
allocation at the top there.
 The loop still needs to take place using the c-string compare methods.

This version of the patch includes that, BUT becomes blocked by the SBuf
API changes as a result.

>
> * Some changed debug lines still quote things for no good (IMO) reason.
> For example:
>
>
>> + debugs(17, 3, clientConn << ": Fetching '" << request->method << ' ' << entry->url() << '\'');
>
>> + debugs(88, 4, '\'' << r->method << ' ' << url << '\'');
>
>> + debugs(88, 4, '\'' << http->request->method << ' ' << http->uri << '\'');
>
>
> There are probably more of these -- I have not checked the whole patch
> this time. You may, of course, insist that this 'quoting' must stay, but
> removing it was one of my suggestions. If needless quoting is removed,
> the above would look like these:
>
>> debugs(17, 3, clientConn << ": Fetching " << request->method << ' ' << entry->url());
>> debugs(88, 4, r->method << ' ' << url);
>> debugs(88, 4, http->request->method << ' ' << http->uri);
>

Oh! Log output quoting was what you meant. Thought you were on about the
"" vs '' string wrappings in the code itself.

Oh well, both are now fixed.

Amos

Received on Fri Apr 11 2014 - 05:33:07 MDT

This archive was generated by hypermail 2.2.0 : Mon Apr 14 2014 - 12:00:12 MDT