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! > > > >> 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. BTW, we use it with NULL content-type argument! I'm not about to dig deep in github to remind myself of the evolution of it, but I guess it's because we only use text/html and set it elsewhere. -- Nick Kew
