Re: [PATCH] SBuf conversion of HttpRequestMethod

On 04/07/2014 09:45 PM, Amos Jeffries wrote:
>
> Add mk-sbuf-strings.awk script to produce a global array of SBuf for the
> string objects of registered methods. This can be re-used to convert
> other enum name arrays as we go forward.
>
> Use an SBuf to store unknown method names "image". Including a few
> string comparison upgrades.

Hi Amos,

> === modified file 'src/HttpRequestMethod.cc'
> --- src/HttpRequestMethod.cc 2013-12-18 00:48:33 +0000
> +++ src/HttpRequestMethod.cc 2014-04-05 12:48:43 +0000
> @@ -47,35 +47,38 @@
> return;
> }
>
> + SBuf str(begin, end-begin);
> +
> for (++theMethod; theMethod < Http::METHOD_ENUM_END; ++theMethod) {
> // RFC 2616 section 5.1.1 - Method names are case-sensitive
> // NP: this is not a HTTP_VIOLATIONS case since there is no MUST/SHOULD involved.
> - if (0 == strncasecmp(begin, Http::MethodType_str[theMethod], end-begin)) {
> + if (0 == image().caseCmp(str)) {
>
> // relaxed parser allows mixed-case and corrects them on output
> if (Config.onoff.relaxed_header_parser)
> return;
>
> - if (0 == strncmp(begin, Http::MethodType_str[theMethod], end-begin))
> + if (0 == image().cmp(str))
> return;
> }
> }
>
> // if method not found and method string is not null then it is other method
> theMethod = Http::METHOD_OTHER;
> - theImage.limitInit(begin,end-begin);
> + theImage = str;
> }
>

This is a performance regression -- we used to allocate the method
buffer only in rare cases of unknown methods. Now we allocate it all the
time. Undo?

> + static SBuf OTHER("METHOD_OTHER");

Make const if possible.

No need to shout and risk conflicts with macros. Consider:

  static const SBuf imageOther = ...

> - char const * image() const;
> + const SBuf image() const;

and

> -inline const char*
> +inline const SBuf
> MethodStr(const MethodType m)

Either return a const reference or remove const. The former avoids
needless refcounting while the latter avoids compiler warnings.

> /** Get a char string representation of the method. */
> - char const * image() const;
> + const SBuf image() const;

The method description is now out of date. I would remove "char" from
that comment to update it.

> - return HttpRequestMethod(m).image();
> + return SBuf(HttpRequestMethod(m).image()).c_str();

and

> - return m.image();
> + return SBuf(m.image()).c_str();

Patched code appears to return a pointer to a buffer inside a temporary
object.

Before the changes, there was an appearance of the same bug but the code
worked correctly because it always returned a pointer to a static
c-string. Patched code 0-terminates a non-static, temporary SBuf. The
code may "accidentally work" in most cases because the underlying
MemBlob allocates enough space to terminate the temporary SBuf without
reallocating its buffer, but we should not rely on that internal
behavior and must not encourage others to use it in contexts where it
will most definitely fail.

> - debugs(33, 3, "clientSetKeepaliveFlag: http_ver = " <<
> - request->http_ver.major << "." << request->http_ver.minor);
> + debugs(33, 3, "http_ver = " << request->http_ver.major << "." << request->http_ver.minor);

Try using this instead:

  debugs(33, 3, request->http_ver);

If that does not work, please use a faster '.' instead of ".".

> - debugs(88, 4, "clientProcessMiss: '" << RequestMethodStr(r->method) << " " << url << "'");
> + debugs(88, 4, "clientProcessMiss: '" << r->method << " " << url << "'");

and

> - debugs(88, 4, "clientProcessOnlyIfCachedMiss: '" <<
> - RequestMethodStr(http->request->method) << " " << http->uri << "'");
> + debugs(88, 4, "'" << http->request->method << " " << http->uri << "'");

and

> - debugs(88, 2, "The reply for " << RequestMethodStr(http->request->method)
> + debugs(88, 2, "The reply for " << http->request->method
> << " " << http->uri << " is " << accessAllowed << ", because it matched '"
> << (AclMatchedName ? AclMatchedName : "NO ACL's") << "'" );

and

> - debugs(85, 2, "The request " <<
> - RequestMethodStr(http->request->method) << " " <<
> + debugs(85, 2, "The request " << http->request->method << " " <<

and

> - debugs(85, 4, "clientProcessRequest: " << RequestMethodStr(request->method) << " '" << uri << "'");
> + debugs(85, 4, request->method << " '" << uri << "'");

and

> - debugs(11, 2, "statusIfComplete: Request not yet fully sent \"" << RequestMethodStr(request->method) << " " << entry->url() << "\"" );
> + debugs(11, 2, "Request not yet fully sent \"" << request->method << " " << entry->url() << "\""

and

> - debugs(44, 3, "peerSelectFoo: '" << RequestMethodStr(request->method) << " " << request->GetHost() << "'");
> + debugs(44, 3, request->method << " " << request->GetHost());

and

> - debugs(44, 3, "peerGetSomeParent: " << RequestMethodStr(request->method) << " " << request->GetHost());
> + debugs(44, 3, request->method << " " << request->GetHost());

and

> - debugs(26, 3, HERE << "'" << RequestMethodStr(request->method) << " " << url << " " << request->http_ver << "'");
> + debugs(26, 3, "'" << request->method << " " << url << " " << request->http_ver << "'");

Same minor optimization/polishing of the debug lines we have to change:
I would drop 'single' and "double" wrapping quotes and use a single ' '
space instead of the null-terminated " " c-string. For example:

  ... '"' << foo << " " << bar << "'" ...

becomes

  ... << foo << ' ' << bar ...

I may have missed more such cases. Please double check.

IMHO, the number of cases where 'quoting' things actually helps is too
small compared to be worth the constant noise introduced by such
"quoting" (and 'quoting' does not reliably solve the problem it tries to
solve anyway!). This quoting approach is one of the things we should be
discussing on the debugging design thread that you think is not needed
at all (AFAICT), so I am forced to plug them in individual patches.

> === added file 'src/mk-sbuf-arrays.awk'
> --- src/mk-sbuf-arrays.awk 1970-01-01 00:00:00 +0000
> +++ src/mk-sbuf-arrays.awk 2014-04-05 07:31:34 +0000
> @@ -0,0 +1,68 @@
> +# tested with gawk, mawk, and nawk.
> +# drop-in replacement for mk-string-arrays.pl.
> +# creates "enum.c" (on stdout) from "enum.h".
> +# invoke similarly: perl -f mk-string-arrays.pl enum.h
> +# --> awk -f mk-string-arrays.awk enum.h

Do we really need two different scripts doing almost the same thing, one
for String and one for SBuf? Can we pass a "type" parameter to a single
.awk script somehow instead (either String or SBuf)?

If src/mk-sbuf-arrays.awk stays, please make sure it does not reference
mk-string-arrays.awk as in the above stale comment.

> - debugs(44, 3, "peerSelect: " << entry->url() );
> + debugs(44, 3, entry->url());

Please do this to provide more/critical information about the entry:

  debugs(44, 3, *entry << ' ' << entry->url());

Thank you,

Alex.
Received on Wed Apr 09 2014 - 15:18:32 MDT