Re: [PATCH] SBuf conversion of HttpRequestMethod

On 14/04/2014 6:20 a.m., Alex Rousskov wrote:
> On 04/10/2014 11:32 PM, Amos Jeffries wrote:
>> I believe the attached patch is complete regarding the method conversion.
>
> The patch looks OK to me. The comments below are minor and can be
> ignored or fixed during commit.
>
>
>> + sb.append(s.rawContent(), s.length());
>> + sb.append(s.rawContent(), s.length());
> ...
>
> There are quite a few of such calls in the patch and they often require
> declaring a temporary SBuf variable. Consider adding
> MemBuf::append(const SBuf &sb) method to handle these better.
>

5 in total converting to both MemBuf and String types. The plan is to
remove those MemBuf and String objects being appended into soon in the
SBuf transition.

>
>> Log output quoting was what you meant.
>
>> + debugs(11, 2, "Request not yet fully sent \"" << request->method << ' ' << entry->url() << '\"' );
>
>> + debugs(11, 3, '\"' << fwd->request->method << ' ' << fwd->entry->url() << '\"');
>
>
> The above two changed lines still have unneeded (IMO) "log output quoting".
>

Oops. Fixed.

>
>
>> + logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " %s %s/%d.%d\" %d %" PRId64 " \"%s\" \"%s\" %s%s:%s%s",
>> clientip,
>> user_ident ? user_ident : dash_str,
>> user_auth ? user_auth : dash_str,
>> Time::FormatHttpd(squid_curtime),
>> - al->_private.method_str,
>> + SQUIDSBUFPRINT(method),
>
> I think it is only a matter of time when Time::FormatHttpd() or another
> function used in this or similar context will call SQUIDSBUFPRINT() to
> format something internally. This problem is out of your patch scope, of
> course, but we should move towards stream << printing to avoid such
> problems and resist adding more printfs.
>

Agreed.

One last build test and this will be applied later today.

Amos
Received on Mon Apr 21 2014 - 17:44:55 MDT