Re: [PATCH] SBuf conversion of HttpRequestMethod

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.

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

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

Cheers,

Alex.
Received on Sun Apr 13 2014 - 18:21:07 MDT