You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Leif Hedstrom <zw...@apache.org> on 2011/02/11 05:08:49 UTC
Re: [DISCUSS] Change SDK return values?
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
Re: [DISCUSS] Change SDK return values?
Posted by Leif Hedstrom <zw...@apache.org>.
On 02/11/2011 06:03 AM, Igor Galić wrote:
>> Right now, we have:
> [snip]
>> ~35 places where we'd need to change that.
> 39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
>
>> Might be worth the effort, I'm at least +0.5
Oh, and all the regression tests too, and other places in the main code
that might use it.
-- Leif
Re: [DISCUSS] Change SDK return values?
Posted by Bryan Call <bc...@yahoo-inc.com>.
I agree with the changes. It would be nice to have standard error
return values across all the functions.
-Bryan
On 02/17/2011 04:11 PM, Leif Hedstrom wrote:
> On 02/11/2011 08:40 AM, Leif Hedstrom wrote:
>> On 02/11/2011 06:03 AM, Igor Galić wrote:
>>>> Right now, we have:
>>> [snip]
>>>> ~35 places where we'd need to change that.
>>> 39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
>>>
>>>> Might be worth the effort, I'm at least +0.5
>>> Still doable.
>>
>> Thinking about this a little more, and I'm still not entirely opposed
>> to #3 (it definitely has some merits), another concern is how people
>> would use this extra information. I mean, if the common pattern is still
> So, I've finished most of this work, and while reading code, and
> discussing on IRC, I think we'll need to make one more change. The issue
> is that a few APIs uses an "int" return code as a boolean, where 1 means
> "success" (e.g. you found a header), and 0 means "failure" (e.g. not
> found). I'd like to propose that we change these to use TSReturnCode as
> well (TS_SUCCESS and TS_ERROR).
>
> The downside to this is that such APIs now requires code changes in any
> existing plugins. It's not impossible to find such cases, but a little
> problematic (since TS_SUCCESS == 0, the logic is inversed). I think I
> can whip up a Perl script that would produce good warnings on areas in
> existing plugin code that the author might need to address the API
> changes. A few example APIs that would change include
>
> TSMgmtStringGet()
> TSIPLookupMatchFirst()
>
>
> Are there any objections or other concerns about this change? It should
> be notable that there are plenty of other API changes, but in most
> cases, the compilers will detect / complain on those. For this, it would
> not.
>
> -- Leif
>
Re: [DISCUSS] Change SDK return values?
Posted by "Alan M. Carroll" <am...@network-geographics.com>.
+1. I think getting a consistent API is worth the short term pain.
Thursday, February 17, 2011, 6:11:29 PM, you wrote:
> So, I've finished most of this work, and while reading code, and
> discussing on IRC, I think we'll need to make one more change. The issue
> is that a few APIs uses an "int" return code as a boolean, where 1 means
> "success" (e.g. you found a header), and 0 means "failure" (e.g. not
> found). I'd like to propose that we change these to use TSReturnCode as
> well (TS_SUCCESS and TS_ERROR).
> The downside to this is that such APIs now requires code changes in any
> existing plugins. It's not impossible to find such cases, but a little
> problematic (since TS_SUCCESS == 0, the logic is inversed). I think I
> can whip up a Perl script that would produce good warnings on areas in
> existing plugin code that the author might need to address the API
> changes. A few example APIs that would change include
> TSMgmtStringGet()
> TSIPLookupMatchFirst()
> Are there any objections or other concerns about this change? It should
> be notable that there are plenty of other API changes, but in most
> cases, the compilers will detect / complain on those. For this, it would
> not.
> -- Leif
Re: [DISCUSS] Change SDK return values?
Posted by Jason Giedymin <ja...@gmail.com>.
A very good point.
+1
On Feb 18, 2011, at 2:39 PM, Leif Hedstrom wrote:
> On 02/17/2011 05:11 PM, Leif Hedstrom wrote:
>> On 02/11/2011 08:40 AM, Leif Hedstrom wrote:
>>> On 02/11/2011 06:03 AM, Igor Galić wrote:
>>>>> Right now, we have:
>>>> [snip]
>>>>> ~35 places where we'd need to change that.
>>>> 39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
>>>>
>>>>> Might be worth the effort, I'm at least +0.5
>>>> Still doable.
>>>
>>>
>>> Thinking about this a little more, and I'm still not entirely opposed to #3 (it definitely has some merits), another concern is how people would use this extra information. I mean, if the common pattern is still
>>
>> So, I've finished most of this work, and while reading code, and discussing on IRC, I think we'll need to make one more change. The issue is that a few APIs uses an "int" return code as a boolean, where 1 means "success" (e.g. you found a header), and 0 means "failure" (e.g. not found). I'd like to propose that we change these to use TSReturnCode as well (TS_SUCCESS and TS_ERROR).
>
>
> Fwiw, here's the changes that would be necessary to eliminate the last remnants of treating "int" as booleans in the APIs:
>
> http://pastebin.com/jWg55hAU
>
>
> As you can see, there's not a whole lot of changes honestly.
>
> Cheers,
>
> -- leif
>
Re: [DISCUSS] Change SDK return values?
Posted by Leif Hedstrom <zw...@apache.org>.
On 02/17/2011 05:11 PM, Leif Hedstrom wrote:
> On 02/11/2011 08:40 AM, Leif Hedstrom wrote:
>> On 02/11/2011 06:03 AM, Igor Galić wrote:
>>>> Right now, we have:
>>> [snip]
>>>> ~35 places where we'd need to change that.
>>> 39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
>>>
>>>> Might be worth the effort, I'm at least +0.5
>>> Still doable.
>>
>>
>> Thinking about this a little more, and I'm still not entirely opposed
>> to #3 (it definitely has some merits), another concern is how people
>> would use this extra information. I mean, if the common pattern is still
>
> So, I've finished most of this work, and while reading code, and
> discussing on IRC, I think we'll need to make one more change. The
> issue is that a few APIs uses an "int" return code as a boolean, where
> 1 means "success" (e.g. you found a header), and 0 means "failure"
> (e.g. not found). I'd like to propose that we change these to use
> TSReturnCode as well (TS_SUCCESS and TS_ERROR).
Fwiw, here's the changes that would be necessary to eliminate the last
remnants of treating "int" as booleans in the APIs:
http://pastebin.com/jWg55hAU
As you can see, there's not a whole lot of changes honestly.
Cheers,
-- leif
Re: [DISCUSS] Change SDK return values?
Posted by Leif Hedstrom <zw...@apache.org>.
On 02/11/2011 08:40 AM, Leif Hedstrom wrote:
> On 02/11/2011 06:03 AM, Igor Galić wrote:
>>> Right now, we have:
>> [snip]
>>> ~35 places where we'd need to change that.
>> 39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
>>
>>> Might be worth the effort, I'm at least +0.5
>> Still doable.
>
>
> Thinking about this a little more, and I'm still not entirely opposed
> to #3 (it definitely has some merits), another concern is how people
> would use this extra information. I mean, if the common pattern is still
So, I've finished most of this work, and while reading code, and
discussing on IRC, I think we'll need to make one more change. The issue
is that a few APIs uses an "int" return code as a boolean, where 1 means
"success" (e.g. you found a header), and 0 means "failure" (e.g. not
found). I'd like to propose that we change these to use TSReturnCode as
well (TS_SUCCESS and TS_ERROR).
The downside to this is that such APIs now requires code changes in any
existing plugins. It's not impossible to find such cases, but a little
problematic (since TS_SUCCESS == 0, the logic is inversed). I think I
can whip up a Perl script that would produce good warnings on areas in
existing plugin code that the author might need to address the API
changes. A few example APIs that would change include
TSMgmtStringGet()
TSIPLookupMatchFirst()
Are there any objections or other concerns about this change? It should
be notable that there are plenty of other API changes, but in most
cases, the compilers will detect / complain on those. For this, it would
not.
-- Leif
Re: [DISCUSS] Change SDK return values?
Posted by Leif Hedstrom <zw...@apache.org>.
On 02/11/2011 06:03 AM, Igor Galić wrote:
>> Right now, we have:
> [snip]
>> ~35 places where we'd need to change that.
> 39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
>
>> Might be worth the effort, I'm at least +0.5
> Still doable.
Thinking about this a little more, and I'm still not entirely opposed to
#3 (it definitely has some merits), another concern is how people would
use this extra information. I mean, if the common pattern is still
if (TSSomeSDKApi(bufp, arg) != TS_SUCCESS) {
then we've added no value. In fact, we've probably made it worse, cause
now we would detect bad input / output parameters the same way as the
API not succeeding for other (good) reasons. The pattern plugins would
use to make this usable (and not worse) is something like
switch (TSSomeSDKApi(bufp, arg, &result)) {
case TS_SUCCESS:
// Do something good with the result., plugin specific
break;
case TS_NOT_FOUND: // (or something)
// We failed the operation, but not because of bad code or bad input.
// This could e.g. be by not finding something we looked for in
the req.
// Action here is plugin specific.
break;
case TS_ERROR;
// This might, or might not be a coding error, but the SDK could
not complete.
break;
case TS_BAD_INPUT:
case TS_BAD_OUTPUT:
// Probably assert or something, because this is a coding error and
// should be fixed, not dealt with at run time (IMO).
break;
default:
// Error cases that we're not aware of.
ink_release_assert(!"zOMG!");
}
So, looking at this, and thinking what an SDK developer would do in the
cases where they are using the APIs wrong, it sort of collapses down to
being very similar to option #1 anyways. The main difference is that a
plugin writer that wants to go out of his way and dealing with bad code
at run-time has an option to do so in #3. But I somehow doubt many
plugin developers would go through all these hoops on every SDK call?
-- Leif
Re: [DISCUSS] Change SDK return values?
Posted by Leif Hedstrom <zw...@apache.org>.
On 02/11/2011 06:03 AM, Igor Galić wrote:
>> Right now, we have:
> [snip]
>> ~35 places where we'd need to change that.
> 39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
>
>> Might be worth the effort, I'm at least +0.5
> Still doable.
Yes, plus any plugin code that we don't have in our SVN repo.
-- Leif
Re: [DISCUSS] Change SDK return values?
Posted by Igor Galić <i....@brainsware.org>.
> Right now, we have:
[snip]
>
> ~35 places where we'd need to change that.
39 if we count it like this: `` ack -l '(!|=)=\s*TS_ERROR' | wc -l ''
> Might be worth the effort, I'm at least +0.5
Still doable.
i
--
Igor Galić
Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
Re: [DISCUSS] Change SDK return values?
Posted by Igor Galić <i....@brainsware.org>.
> 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.galic@brainsware.org
URL: http://brainsware.org/