Change location types?

[Bryan Call <bc...@yahoo-inc.com>]

I have seen people writing plugins pass incorrect locations to APIs.  
For example you can pass a client request header to an API that wants 
the url without any errors from the compiler.

Looking at the three APIs below it is hard to tell the location/offset 
really point to:

   tsapi TSReturnCode TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer* 
bufp, TSMLoc* offset);
   tsapi TSReturnCode TSHttpHdrUrlGet(TSMBuffer bufp, TSMLoc offset, 
TSMLoc* locp);
   tsapi const char* TSUrlSchemeGet(TSMBuffer bufp, TSMLoc offset, int 
*length);

In a specific case that happened last week a person was taking bufp and 
offset from TSHttpTxnClientReqGet() and passing the values to 
TSUrlSchemeGet().  Since it didn't give him compile errors he only saw 
the problem when the server was under load and it would core dump.

One way to fix this would be to change locations to specific types.  If 
we want to take it a step further the buffer and location could be 
combined and given a specific type simplifying the APIs.

Example of changing to specific location types:
   tsapi TSReturnCode TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer* 
bufp, TSMRequestLoc* offset);
   tsapi TSReturnCode TSHttpHdrUrlGet(TSMBuffer bufp, TSMRequestLoc 
offset, TSMUrlLoc* locp);
   tsapi const char* TSUrlSchemeGet(TSMBuffer bufp, TSMUrlLoc offset, 
int *length);

Example of combining buffer and location and making specific types (IMO 
cleaner):
   tsapi TSReturnCode TSHttpTxnClientReqGet(TSHttpTxn txnp, TSRequest 
*request);
   tsapi TSReturnCode TSHttpHdrUrlGet(TSRequest request, TSUrl *url);
   tsapi const char* TSUrlSchemeGet(TSUrl url, int *length);

-Bryan

Re: Change location types?

[Leif Hedstrom <zw...@apache.org>]

On 02/28/2011 11:11 AM, Bryan Call wrote:
> I have seen people writing plugins pass incorrect locations to APIs.  
> For example you can pass a client request header to an API that wants 
> the url without any errors from the compiler.
>
> Looking at the three APIs below it is hard to tell the location/offset 
> really point to:
>
>   tsapi TSReturnCode TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer* 
> bufp, TSMLoc* offset);
>   tsapi TSReturnCode TSHttpHdrUrlGet(TSMBuffer bufp, TSMLoc offset, 
> TSMLoc* locp);
>   tsapi const char* TSUrlSchemeGet(TSMBuffer bufp, TSMLoc offset, int 
> *length);
>
> In a specific case that happened last week a person was taking bufp 
> and offset from TSHttpTxnClientReqGet() and passing the values to 
> TSUrlSchemeGet().  Since it didn't give him compile errors he only saw 
> the problem when the server was under load and it would core dump.

Yes, this is indeed a pretty weak and confusing arrangement we have 
right now, with TSMLoc being the kitchen sink :/. The one thing that 
comes to mind though, as we discussed on IRC, is that separating 
"request" and "response" types probably doesn't make sense, because it 
could really complicate and duplicate many APIs.

Btw, and not that it's an excuse not to do this, but as of v2.1.6, the 
implementation of these APIs have (by default) a much better validation 
of input parameters. E.g. they will do asserts like

   sdk_assert((sdk_sanity_check_mime_hdr_handle(mh_mloc) == TS_SUCCESS) ||
              (sdk_sanity_check_http_hdr_handle(mh_mloc) == TS_SUCCESS));


Which means, if you pass in a TSMLoc of the "wrong" type to a particular 
API, you will notice immediately. This was part of the major cleanup 
efforts that we did recently to the APIs. It's quite likely there are 
areas for improvements here too, but the "foundation" is there for 
making the APIs safer and easier to use.


>
> One way to fix this would be to change locations to specific types.  
> If we want to take it a step further the buffer and location could be 
> combined and given a specific type simplifying the APIs.

Yeah, I think it makes sense to "combine" the two together, as long as 
there is not some significant number of APIs that only requires one or 
the other. Anything to make the APIs easier is a Good Thing :).

Cheers!

-- leif