> 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.
+++1
> 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) { ...
Right now, we have:
i.galic@panic ~/Projects/asf/ats-plugins (svn)-[plugins:1069751] % ack -l
'==\s*TS_ERROR' | wc -l
1
i.galic@panic ~/Projects/asf/ats-plugins (svn)-[plugins:1069751] %
../trafficserver
i.galic@panic ~/Projects/asf/trafficserver (svn)-[trunk:1068985] % ack -l
'==\s*TS_ERROR' | wc -l
34
i.galic@panic ~/Projects/asf/trafficserver (svn)-[trunk:1068985] %
~35 places where we'd need to change that.
Might be worth the effort, I'm at least +0.5
> 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
>
>
--
Igor Galić
Tel: +43 (0) 664 886 22 883
Mail: i.ga...@brainsware.org
URL: http://brainsware.org/
