Libelektra: Each Error should be mapped to a different error code

[PhilippGackstatter]

As mentioned in #2979 many functions return -1 for multiple errors. Take keySetName as an example. The documentation states

Return values

size | in bytes of this new key name including ending NULL
0    | if newName is an empty string or a NULL pointer (name will be empty afterwards)
-1   | if newName is invalid (name will be empty afterwards)
-1   | if key was inserted to a keyset before

Two different errors are mapped to -1, making them indistinguishable for the caller. This is especially problematic for bindings that map these error codes to Exceptions or some Error structure. The bindings might then throw an InvalidName Exception because -1 was returned, but the real problem was that the key was in a keyset. This only leads to confusion for developers.

Error codes should probably use -1, -2, and so on (or any other mechanism that allows for explicitness) to make exact matching for the caller possible.

[markus2330]

Thank you for the proposal!

[markus2330]

I agree that mapping to different negative numbers should have no negative side-effect (comparing using < 0 on "any error" is trivial) but helps in some cases (at least in the case you had in the binding and a similar case I already had in the python binding) where the different error matters.

How many places are effected (next to keySetName)?

[PhilippGackstatter]

How many places are effected (next to keySetName)?

Also affected

• keyGetMeta
• keyAddBaseName
• keyAddName
• keySetBaseName
• keyGetBinary
• keyGetString
• keySetBinary

Affected, if null pointer and memory errors should be mapped to different error codes

• keyCopyAllMeta
• keyCopyMeta

Maybe affected, needs discussion

• keyGetFullName (returns -1 on Null pointers or if maxSize is 0 or larger than SSIZE_MAX)
• keyGetBaseName (same as above)
• keyGetName (same as above)
• keyString, maybe? It could return error codes instead of "(null)" and "(binary").

[markus2330]

Also affected

We should fix all of them (with consistent error values).

Affected, if null pointer and memory errors should be mapped to different error codes

I think there is little benefit from this.

returns -1 on Null pointers or if maxSize is 0 or larger than SSIZE_MAX

Is both API misuse, so same return value is ok.

keyString, maybe? It could return error codes instead of "(null)" and "(binary").

This is on purpose so that a printf("%s", keyString(k)) is safe.

[stale[bot]]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions :sparkling_heart:

[markus2330]

@kodebach did you have some new insight here while you did #3102?

[kodebach]

No real insights here. Some of the functions got removed. Some of the error cases went away e.g. key->key cannot be NULL anymore, it is allocated in keyVNew and only freed in keyDel (keyClear frees it but also allocates a new buffer).

Also an important feature of the new key name manipulation methods is: The key is either changed as intended or left untouched. "(name will be empty afterwards)" won't happen unless this was the explicit intention of the caller.

In most instances, the caller of a key name manipulation method already knows for sure that the call will succeed and therefore won't even check the return value.

---

A few more general points:

• In many cases, functions return the size of a key name. Since / is now the shortest possible key name, 0 could be used as an error code in these functions. Some of these also only have one error case. That's why I thought about using size_t instead of ssize_t.
• In some places the documentation (not updated yet) still mentions an error code for allocation errors. AFAIK we agreed that error codes for allocation errors don't make much sense and should just assume that malloc can't fail. So I currently don't check the return values of any of the allocations, so those error codes aren't relevant anymore either.
• It also really depends, how far you want to go with error codes. Is "Invalid Key Name" always one code, or do we use separate keys for "Illegal Escape Sequence", "too many .. parts", etc.?

IMHO "every error should have its own code" is a good idea, but one error code should always be just "precondition failed" and there should be a reasonable set of preconditions.

IMO this would not be a good API:

 * @retval -1 if the `key` had no name
 * @retval -2 on NULL pointers
 * @retval -3 if `key` was inserted to a keyset before
 * @retval -4 on allocation errors
 * @retval -5 if `name` is invalid
 * @return otherwise, new size of key name
 */
ssize_t keyAddName (Key * key, const char * name)

Instead, I would suggest:

 * @pre `key != NULL && name != NULL` and `key` is a valid non-readonly Key
 *
 * @retval -1 preconditions failed
 * @retval -2 `name` is invalid
 * @return otherwise, new size of key name
 */
ssize_t keyAddName (Key * key, const char * name)

(Please ignore that some of the error cases, don't make sense anymore)

I removed the allocation error code, as noted above.
Combining "the arguments are != NULL" into one code was already done, and adding the -1 and -3 cases is also reasonable, because the caller will know whether these conditions are met in almost all cases.

A big problem with having lots of error codes this deep in the core is that library functions either need to combine the error codes anyway or they will have loads of error codes that nobody bothers to check.

---

If we want detailed error codes for "Invalid Key Name", I would suggest we add a public wrapper for elektraKeyNameValidate that returns these error codes, instead of complicating the return values of the more commonly used functions.

[kodebach]

Also a very important note IMO: Using any return values other than 0 or 1 in a function that appears to be a boolean test (e.g. keyIsBinary) is very bad. keyIsBinary (NULL) is currently treated as true in a simple if (keyIsBinary (key)).

[PhilippGackstatter]

I agree that preconditions can be combined into a single error code. A developer doesn't need to differentiate the return value when passing NULL or "" to keySetName, it can be the same: precondition failed. And this is already the case (for this function, see introductory example), where 0 is the return value.

The keySetName example mainly came up because I simply wanted to pass the two different errors on to Rust binding users, but I didn't actually think about whether differentiating between those errors is reasonable or necessary, because I didn't write the actual function.
So if the developer of keySetName decides that these are two separate errors, then there should be two different error codes. If not, they should be combined into one error and error code. But as it is right now, the documentation implies there are two errors but provides just one error code. That discrepancy should be fixed.
And I think the developer of keySetName should answer whether developers will actually want to handle the different error codes or not.

[markus2330]

Yes, I fully agree. And I like @kodebach's idea that we should add an extra function for validation instead of making the API more complicated. This function could be called only setting the name failed, so it would be no performance overhead in the non-erroneous case. And probably we should also add a function to check if the key is in a key set. These functions, however, also could be added post-1.0.

So I would say, we can close the issue. A full review of the API is needed anyway to really get everything nicely consistent.