[API REVIEW] Modify string ownership for TSRedirectUrlSet()

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

Hi all,

as part of the v5.0.0 API fixup’s, I’d like to change the string ownership “contract” for TSRedirectUrlSet(). Today, the incoming URL string is basically strdup()’ed, and in most use cases I can think of, it’d be more logical to stick to how we do other similar APIs, which is to assume the caller has allocated the memory. This also significantly improves efficiency for many use cases. For example, a typical plugin use case would be something like this:

          url_str = TSUrlStringGet(mbuf, url, &url_len);
          TSRedirectUrlSet(txn, url_str, url_len);
	  TSfree(url_str);  // I’d like to eliminate this!

In the current implementations, this causes malloc() / strcpy() of the url_str twice; once in the plugin, and then once again in the TSRedirectUrlSet() API. This is a waste :). I understand this breaks implementations using this API, but I’m still on the mindset that we should take this opportunity to improve on the APIs in v5.0.0. We haven’t done so since v2.0, and we’ve learned a lot since then. We will provide tools (such as the existing perl script) that helps people migrate their plugins, and of course, all core plugins will be fixed.

I filed a Jira on this, at

	https://issues.apache.org/jira/browse/TS-2693


— Leif

Re: [API REVIEW] Modify string ownership for TSRedirectUrlSet()

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

On Apr 2, 2014, at 9:46 AM, Leif Hedstrom <zw...@apache.org> wrote:

> Hi all,
> 
> as part of the v5.0.0 API fixup’s, I’d like to change the string ownership “contract” for TSRedirectUrlSet(). Today, the incoming URL string is basically strdup()’ed, and in most use cases I can think of, it’d be more logical to stick to how we do other similar APIs, which is to assume the caller has allocated the memory. This also significantly improves efficiency for many use cases. For example, a typical plugin use case would be something like this:
> 
>          url_str = TSUrlStringGet(mbuf, url, &url_len);
>          TSRedirectUrlSet(txn, url_str, url_len);
> 	  TSfree(url_str);  // I’d like to eliminate this!


So I’ve spent some more time on this, and instead of the above, I’d like to propose the following:

1. We leave TSRedirectUrlSet() / Get() as is (as far as functionality goes). This makes this proposal both API and binary compatible with v4.2.x.

2. We mark them as Deprecated, removing the old API in 6.0.0.

3. We add TSHttpTxnRedirectSet() / Get() , with the new string ownership as described above. This naming convention is much better aligned with the rest of our APIs.


I left the URL string lengths as int type for now, we’ll discuss at the Summit about TS-2514, changing our usage of “int” to more appropriate size_t etc.

Cheers,


— Leif


  /**
     This is a generalization of the TSHttpTxnFollowRedirect(), but gives finer
     control over the behavior. Instead of using the Location: header for the new
     destination, this API takes the new URL as a parameter. Calling this API
     transfers the ownership of the URL from the plugin to the core, so you must
     make sure it is heap allocated, and that you do not free it.

     Calling this API implicitly also enables the "Follow Redirect" feature, so
     there is no rason to call TSHttpTxnFollowRedirect() as well.

     @param txnp the transaction pointer
     @param url  a heap allocated string with the URL
     @param url_len the length of the URL
  */
  tsapi void TSHttpTxnRedirectUrlSet(TSHttpTxn txnp, const char* url, int url_len);
  tsapi TS_DEPRECATED void TSRedirectUrlSet(TSHttpTxn txnp, const char* url, const int url_len);

  /**
     Return the current (if set) redirection URL string. This is still owned by the
     core, and must not be free'd.

     @param txnp the transaction pointer
     @param url_len_ptr a pointer to where the URL length is to be stored

     @return the url string
  */
  tsapi const char* TSHttpTxnRedirectUrlGet(TSHttpTxn txnp, int* url_len_ptr);
  tsapi TS_DEPRECATED const char* TSRedirectUrlGet(TSHttpTxn txnp, int* url_len_ptr);