Re: [PATCH] protocol_t upgrade

On 02/27/2011 06:12 PM, Amos Jeffries wrote:
> This begins the libanyp.la SourceLayout changes by moving the protocol_t
> type code to stand-alone files inside its namespace.
>
> On the most part there are no behaviour changes. The automatic
> generation of the enum->text string produces an upper-case strings
> array, so explicit down-casing is added for the obvious places such as
> scheme:// syntax.

> + static char out[BUFSIZ];
> + *out = '\0';
> + if (in != NULL) {
> + int p = 0;
> + for (; p < BUFSIZ && in[p] != '\0'; ++p)
> + out[p] = xtolower(in[p]);
> + out[p] = '\0';

The last line will write outside the "out" buffer boundary if "in" is
longer than BUFSIZ. The first assignment would be a waste of cycles if
"in" has characters in it which is probably the common case. You
probably want to reshuffle to have just one (and correct) termination
assignment at position p, where p is always correct and smaller than BUFSIZ.

I could not find where this function is used in the patch. Is there some
code missing from the patch?

> +/// Display the Protocol Type (in upper case).
> +inline std::ostream &
> +operator <<(std::ostream &os, ProtocolType const &p)
> +{
> + if (p < PROTO_UNKNOWN)
> + os << ProtocolType_str[p];
> + return os;
> +}

Should we print something when p is PROTO_UNKNOWN or worse? Where do we
use this operator?

> - if (request->protocol == PROTO_HTTP)
> + else if (request->protocol == AnyP::PROTO_HTTP)
> return httpCachable(method);
>
> - if (request->protocol == PROTO_GOPHER)
> + else if (request->protocol == AnyP::PROTO_GOPHER)
> return gopherCachable(request);
>
> - if (request->protocol == PROTO_CACHEOBJ)
> + else if (request->protocol == AnyP::PROTO_CACHEOBJ)

Your call, but I would not change "if" to "else if" here. The change
makes understanding the code more difficult and is inconsistent with the
rest of that function.

> - CbDataList<protocol_t> *q = new CbDataList<protocol_t> (urlParseProtocol(t));
> - *(Tail) = q;
> - Tail = &q->next;
> + for (int p = AnyP::PROTO_NONE; p < AnyP::PROTO_UNKNOWN; ++p) {
> + if (strcasecmp(t, AnyP::ProtocolType_str[p]) != 0) {
> + CbDataList<AnyP::ProtocolType> *q = new CbDataList<AnyP::ProtocolType>(static_cast<AnyP::ProtocolType>(p));

The above changes the behavior from creating a PROTO_NONE-based
CbDataList to silently skipping some of the unknown protocols. Do we
check for the unknown protocols in the caller? If not, will this code
change cause any user-visible changes?

Also, urlParseProtocol() does not recognize some of the protocols for
which the strcasecmp test will succeed, right? Should urlParseProtocol()
be extended to be consistent with the new enumeration logic above?

When a similar change was done for request methods, we ended up creating
an HttpRequestMethod class with a few useful methods. IIRC, the primary
motivation was correct handling of unknown methods -- we needed a place
to store the unknown method image/string. Do we need any need to handle
unknown protocols by storing their image/string?

Thank you,

Alex.
Received on Mon Feb 28 2011 - 21:56:10 MST