On Tue, Mar 25, 2014 at 8:39 AM, Leif Hedstrom <zw...@apache.org> wrote:
> > 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. > > +1. We don't want to tie ourselves down to bad API's, but we also don't want to strand users either. You should be able to use the 4.2.x LTS branch for ~1 year still so nothing should be forcing your upgrade hastily. > 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 > >
