On Thu, 2010-10-28 at 08:38 -0600, Leif Hedstrom wrote:
> while looking at some issues we're having with InkAPI plugins, the issue
I saw the message yesterday on the changing of the plugin A{BP}I in a
non-compatible way in order to support 64 bit lengths. (i.e. option 2).
I hope if you do that you also change the ATS major version to 3. People
expect compatibility across minor releases, and rightfully so as that is
the industry convention.
As long as you are making breaking-changes I really hope you'll take it
further than just the 64 bit change.. here are a few suggestions based
on my experience with the current SDK:
#1 - address the utter lack of type safety. e.g.: vconnection callback
functions should not be the same as transaction callback functions.. and
things like INKIOBuffer and INKIOBufferData and INKIOBufferBlock are all
typedef'd to void *. I understand multiplexing void *edata in the
callback function (though I don't think you should do that either), but
essentially saying at the compiler level "any pointer to
INKVConnClose()" will be fine is just shooting yourself in the foot.
Indeed, this was the source of my post the other day dealing with
HttpTxnIntercept() failing - I had passed a pointer of the wrong type to
some function that could easily have been well-typed and things just
went off the rails from there. In various guises this has been behind
almost all my "getting-started" frustrations with the plugin
architecture.
#2 - A simple and common operation like "get response content-type" is
really a small mountain of code. Consider providing easy wrapper
functions for getting and setting of single valued headers (have them
toss errors when they aren't single valued) and response codes. They can
still be zero-copy value/length oriented and reference counted - just
make it opaque.. e.g:
INKHeader headerval;
if (INKGetResponseHeader (&headerval, "Content-Type") == 0) {
LocalCode_ProcessContentType(headerval.str, headerval.len);
INKReleaseResponseHeader(&headerval);
}
#3 - INKHttpHdrDestroy(bufp, hdr) - requires the same bufp be passed as
at was used at creation time.. this seems to be an error prone
expectation of caller, just store it in hdr. I think there are other
examples like this..
#4 - most things are INKCamelCase, but some are not: INKmalloc.
Personally, this fills me with doubt every time I start to type INK..
because I cannot remember what the pattern is. There are lots of other
naming inconsistencies - for example INKMBufferCreate() creates an
INKMBuffer, INKHttpParserCreate() creates a INKHttpParser, and
INKHttpHdrCreate() creates a INKHttpHdr^H^H^H^H^H^H^H^H^H INKMLoc. Say
what? All that little stuff just makes the SDK frustrating to use and
violates expectations - and violated expectations lead to bugs
(especially when everything is a void *).
#5 - memory request pools (ala apache http server) would be an obvious
and important addition for plugins to use.
- free this string..
hope this helps.
-Patrick
