On Mar 24, 2014, at 3:26 PM, James Peach <jpe...@apache.org> wrote:
> On Mar 24, 2014, at 9:45 AM, Leif Hedstrom <zw...@apache.org> wrote:
>
>> 3. In order to make the TSHttpTxnErrorBodySet() API easier to use, I’ve
>> changed the content type argument to not transfer ownership. The common
>> (well, exclusive) pattern using this API used to be
>>
>> TSHttpTxnErrorBodySet(txnp, body, body_len, TSstrdup(“plain/html”));
>>
>>
>> This just seems like an overly generalized case, for a problem that doesn’t
>> exist (clearly we can all manage some static strings ;). The new API is
>> simply:
>>
>> TSHttpTxnErrorBodySet(txnp, body, body_len, “plain/html”);
>
> I don't like how this mixes memory handling in the same API. I think that the
> consistent form of this API should be:
>
> TSHttpTxnErrorBodySet(txnp, body, body_len, content_type,
> content_type_len);
>
> I agree that a string literal is probably the most common usage, but we
> should make it behave the same as other APIs.
William Bardwell gave the same feedback. This API is already inefficient, and
hopefully rarely used, so one more malloc won’t kill us. So, the API will be
TSHttpTxnErrorBodySet(txnp, body, body_len, content_type,
content_type_len);
with a consistent transfer of ownership of both “body” and “content_type”, both
will be free()’d when the core no longer needs them.
>
>> 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).
There are no use cases in the current APIs / plugins which depends on the log
tag expansion, so I was hoping to keep these two API changes separate patches
(or I’d go insane). I also plan on making some improvements to the body factory
as part of the new API additions.
>
>> So, I’d like to open this up for discussions and concerns. I think 3) and 4)
>> are the primary candidates for concerns and objections. I’m willing to
>> compromise either or both if there are major concerns (for example, is it
>> worth the “cost” of changing the API in 3) ? I think it is, but I can easily
>> be persuaded otherwise).
>>
>> Finally, I’ve attached the current patch to the Jira:
>
> Which Jira was that?
https://issues.apache.org/jira/browse/TS-2657
— Leif
