keyString() returned value

[lawli3t]

This is probably something that has come up numerous times, but I still found it worthy of discussion, especially because I don't know much about the history of this function, as well as the implications of eventual changes to this function.

When using keyString() on empty / binary values the return values are the literal strings (null) / (binary) [1]. This seems very awkward and unintuitive from a user's perspective.

There are several reasons why this is suboptimal.

• If a user were to store the value (null) for some reason, he could never be sure whether the Key he just queried has the value (null) or whether he tried getting a NULL value from a string.
• It is very unintuitive from a users perspective that string literals are returned for those special / error cases
• From a puritan's point of view this just seems very inelegant

Because the return value of the function is const char*, there are not so many possibilities for a change to this behaviour, as every possible return value - except for a NULL pointer - could just be the value of the string of the Key. Possible changes to this behaviour would be:

• Returning a NULL pointer for both cases seems to be the straightforward way, which would solve the problems posited above and make the behaviour of the function more intuitive.
• Other functions that return pointers make use of errorKeys, that contain additional information in case of errors. keyString() seems like a very elementary function, that should be kept as simple as possible. Also adding a parameter just for this error handling and breaking backwards compatibility in another way seems to be way too big of a change for this function. I just included this option for completeness sake.
• @markus2330 talked about maybe introducing a thread-local error variable [2]. This function might be a good candidate for this change aswell.

Of course there are several things to consider before making such a fundamental change

• Do the benefits of this change outweigh the possible breakage stemming from this change?
• How much code would really be affected by this change? Are the return values in cases of error really checked that often? Do most people not use keyIsBinary / keyIsString anyway before calling keyString / keyValue ?
• How many people would really store the values (null) and (binary) and be affected by the current situation?

[1] https://doc.libelektra.org/api/latest/html/group__keyvalue.html#ga880936f2481d28e6e2acbe7486a21d05
[2] #3737 (comment)

[mpranj]

I tend to agree with your reasoning.

Background: For some historic reason keyString() was written like this, I assume it made sense at the time. I'm guessing that it was easier to use it like this when printing etc., without doing any checks (e.g. for NULL). I think @markus2330 knows more about this.

The problem is distinguishing between errors. How to warn/hint users if they call keyString() on e.g. a binary key. The call is obviously an error, but you could give them the info that they can use keyGetBinary().

Using (binary) might not be intuitive, but it at least gives a hint on what's going on.

[lawli3t]

The problem is distinguishing between errors. How to warn/hint users if they call keyString() on e.g. a binary key. The call is obviously an error, but you could give them the info that they can use keyGetBinary().

Yes, that's also the problem I had - it is actually really hard to find a proper solution for this in my opinion. I tried to explore some options but they come attached with problems of their own.

[mpranj]

This issue is also related to #2296. Actually #3797 might be a subset of #2296.
Maybe we can find a solution to both issues?

[markus2330]

When using keyString() on empty / binary values the return values are the literal strings (null) / (binary) [1]. This seems very awkward and unintuitive from a user's perspective.

Yes, I agree.

Returning a NULL pointer for both cases seems to be the straightforward way, which would solve the problems posited above and make the behaviour of the function more intuitive.

The idea was that the person has a guarantee that (s)he gets a string. If you are okay with null pointers, you can use keyValue instead.

@markus2330 talked about maybe introducing a thread-local error variable [2]. This function might be a good candidate for this change aswell.

Yes, then we could return some string but still indicate the error.

Do the benefits of this change outweigh the possible breakage stemming from this change?

Breakage is something we do not need not consider now (except in the sense that somehow it must be possible for us to fix our code base). Now is the time to do any such changes so that 1.0 has better usability.

How much code would really be affected by this change?

Which change do you mean? The change of returning null would affect many places (everywhere keyString gets printed).

Are the return values in cases of error really checked that often?

I was thinking they are not, thus I made keyString to always return a string. Then the string can be safely passed along, without doing any error checks.

Do most people not use keyIsBinary / keyIsString anyway before calling keyString / keyValue ?

I don't think they do before calling keyString because it handles the case already.

How many people would really store the values (null) and (binary) and be affected by the current situation?

As said: it does not matter what people currently might do, 1.0 is a huge breaking change anyway. And if someone really relies that (null) gets returned, the code needs to be adapted. It would be nice if mention such things in the 1.0 Changelog. See #3819. From 1.0 on, however, people should be able to rely that (null) (or whatever it will be) gets returned.

[kodebach]

On the one hand always returning a string allows things like strlen(keyString(k)) == 0 too check for an empty string. But at the same time people might expect that strlen(keyString(k)) < MAX_LEN also works, but this might silently break code down the line, if k is binary and strlen("(binary)") < MAX_LEN.

I think the best solution would be:

1. Obviously just return the string value, if the key is a non-empty string key.
2. Return NULL, if the key is binary (no matter what the value is)
3. Return an empty string, otherwise, i.e. when key contains an empty string or k->data.c == NULL, but the key is not marked as binary.
   This way you'll probably get a segfault for unexpected binary keys (unless you check for them), but the common cases are still easy to handle without much checking. I prefer the segfault over returning some fixed string value, because then bugs don't cause silent problems.

An alternative would be to always store a zero byte after all key values, even if they are binary (might be done already). Then we can safely return k->data.c == NULL ? "" : k->data.c. It may contain incomplete data and the MAX_LEN problem from above still applies, but there are no segfaults and you don't get return values that have nothing to do with the actual data.

This issue is also related to #2296.

I don't think so, see #2296 (comment)

[lawli3t]

further discussion in #3973