trafficserver-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leif Hedstrom <zw...@apache.org>
Subject Re: [DISCUSS] Change SDK return values?
Date Fri, 11 Feb 2011 04:08:49 GMT
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


Mime
View raw message