On Mar 25, 2014, at 12:16 AM, Nick Kew <n...@webthing.com> wrote:
> On Mon, 2014-03-24 at 20:39 -0600, Leif Hedstrom wrote:
>
>> with a consistent transfer of ownership of both “body” and “content_type”,
>> both will be free()’d when the core no longer needs them.
>
> ISTR that being a minor "gotcha". There's a comment in the
> Ironbee code about the body being freed so we have to copy it
> first: I think that surprised me when I first encountered it!
Yes, it’s a pretty unusual API in ATS. The alternative is to force the plugins
to retain ownership. For dynamically allocated memory, that means freeing it in
(I think) a TS_HTTP_TXN_CLOSE_HOOK, passing along the pointers as cont data…
>
>>>
>>>> 4. The TSHttpTxnErrorBodySet() also imposes the body to be sent through
>>>> the body factory, for log field expansion. For starters, this is
>>>> incredibly inefficient , as well as imposing an 8KB size limit. This is
>>>> something I intend to ameliorate soon by improvements to the body factory.
>>>> However, this is an unnecessary inefficiency IMO; As part of the body
>>>> factory improvements, I also plan on exposing the body factory “log field”
>>>> expansion as a generic API. This would allow any plugin to decide if they
>>>> want to go through the pains of body factory expansions or not (and not
>>>> limited to just error pages, e.g. Header Rewrite would use this new API).
>>>
>>> I'd be pretty surprised if anyone was taking advantage of the body factory
>>> stuff. One suggestion is to add a flags field (with valid values checking).
>>> For example,
>>>
>>> TSHttpTxnErrorBodySet(txnp, 0, body, -1, content_type, -1);
>>> TSHttpTxnErrorBodySet(txnp, TS_BODY_INTERPOLATE_LOG_TAGS, body, -1,
>>> content_type, -1);
>>
>> I would really prefer to keep this separate, such that the body factory
>> expansion shenanigans happens separately (separate APIs). That way, some
>> plugins can avoid at least one malloc(), since the new body factory
>> expansion API would not transfer ownership of the buffer (so the template
>> can be e.g. a plugin “global”, or stored in remap instance data).
>
> An API change like that affects existing plugins and could leave us
> needing some ugly #ifdef crap to support both before- and after- TS
> versions. Can I make a plea to avoid that: maybe a new function name,
> and migrate the old one as a #define wrapper for the new? That applies
> equally to the above with flags or to just adding the c-l-len argument.
We have no precedence with creating new APIs and keeping old ones around, and I
really feel that just adds onto an already confusing API (and somewhat defeats
the purpose of this cleanup). But I also understand your concerns.
Note that we’re making no backwards compatibility promises for v5.0.0, we’ve
already broken other APIs, and we’re taking this opportunity to fix others. For
a partial list of what is on the plan, see
http://s.apache.org/3eH
I need to go through this, triage it and perhaps add some more bugs. It’ll be
documented properly in the “Upgrade to v5.0.0” docs, and also update the perl
scripts. Anyone else interested in cleaning up the APIs, should take a look at
the above (and file more Jiras :).
So I suspect that you’ll be having to #ifdef anyways, if you wish to support
both old and new versions of ATS (outside the main tree).
That much said, if there’s a strong opinion on keeping compatibility in this
particular API, my vote would be to just leave out the content_type_len. I’m
not sure where this leaves us right now with this bug….
— Leif
