On 12/08/2010 03:07 PM, Leif Hedstrom wrote:
Hi all,
there's been some discussions about cleaning up how our APIs (SDK)
deals with return values. Right now, it's sort of a mess, with
different APIs using different standards (e.g. return a POD data type,
return a TSReturnCode, return an int etc.). There are a few
suggestions on the table, each which has pros and cons. I'd like to
start the discussion on this, so we can come to a consensus and make
any changes sooner rather than later.
So, we had this discussion a while ago, and the consensus was to try to
unify the code on clear cut return codes. I've spent some time going
through a bunch of the APIs, and I've come to realize that many of them
could only "fail" in a debug build. Basically, the could would look like
e.g.
TSMLoc
TSMimeHdrFieldNextDup(TSMBuffer bufp, TSMLoc hdr, TSMLoc field)
{
if ((sdk_sanity_check_mbuffer(bufp) != TS_SUCCESS) ||
((sdk_sanity_check_mime_hdr_handle(hdr) != TS_SUCCESS) &&
(sdk_sanity_check_http_hdr_handle(hdr) != TS_SUCCESS))
|| (sdk_sanity_check_field_handle(field, hdr) != TS_SUCCESS)) {
return (TSMLoc)TS_ERROR_PTR;
}
This is the "error" case for this API, for all other cases, it'll return
a TSMLoc, either a valid pointer ("field found") or TS_NULL_MLOC ("not
found"). Here's the catch: This error case would only ever trigger in a
debug build, in a release build the sdk_sanity_check_XYZ are always no-ops.
To me, this is really undesirable behaviour, with the APIs having
completely different semantics depending on debug or release build (in
debug build we'd return e.g. a mystical TS_ERROR_PTR (or some other
special error value), which the plugin has to deal with or perhaps
crash), while in a release build the API would instantly crash. I'd
honestly rather prefer the "fail and crash quick and fast", particularly
in debug build.
To finish this up, and to be able to complete all these changes, I'd
like to discuss a few additional options to deal with this, please let
me know what you think. Some options are:
1) Change all sdk_sanity_check_XYZ() to be the equivalent of
ink_assert(), and change the usage accordingly. This means that a large
number of APIs can stay the way they are today (e.g. return TSMLoc),
and the return value can only be either "success" (not-NULL) or
"failure" (NULL). The usage of such APIs then becomes a simple
if (loc =TSMimeHdrFieldNextDup(bufp, hdr, field)) { ...
The main difference from current APIs being that these APIs can never
return their special error case values, like TS_ERROR_PTR, which would
only (currently) happen in debug builds anyways.
2) Keep it as is, but change the APIs to return a TSReturnCode. This
unfortunately means that usage of such API would have to be something like
if ((TSMimeHdrFieldNextDup(bufp, hdr, field, &loc) == TS_SUCCESS)
&& (loc != TS_NULL_MLOC)) { ...
Technically, plugins already have to do something similar, but this
would at least be clear that you only check the return value against
TS_SUCCESS / TS_ERROR, and then assure that the function actually
succeeded. This simplifies the APIs a bit, since you don't need to know
each APIs special error value.
2.5) Change all sdk_sanity_check_XYZ to actually be used in release
builds as well as debug build. This would probably be in addition to #2,
and it requires the same pattern (i.e. it's not enough to check the
return value).
3) One last option is to extend TSReturnCode to have more states than
TS_SUCCESS and TS_ERROR. E.g. let it have a state like TS_BAD_INPUT and
TS_BAD_OUTPUT. I looked into this before, and the problem with this
approach is that it risks breaking a *lot* of code in a way where it's
very difficult to detect (i.e. compilers won't be able to find it). E.g.
a plugin like
if (TSSomeSDKApi(bufp, argv) == TS_ERROR) { ...
wouldn't detect the new states properly. All such code would have to be
changed to either check on all possible return values, or more likely,
change to
if (TSSomeSDKApi(bufp, argb) != TS_SUCCESS) { ...
This becomes a much bigger tasks, because now all APIs that already use
TSReturnCode (there's a lot of those) are also affected. But it is
pretty clean...
I personally like #1, and perhaps #3 (but #3 is a ton more risk and a
lot more work).
Thoughts? I'd like to finish this task next week (it's a beast), so
input is much appreciated.
Cheers,
-- leif
