Low-Level API Improvements

[PhilippGackstatter]

While writing the Rust Bindings (#2730), @markus2330 asked me to look for improvements in the C-API, so here are my findings.

• Some typos, will be fixed in doc: Fix typos in documentation #2978

• As already mentioned in 2730, many methods return -1 for multiple errors. The caller cannot distinguish between errors. Examples are keySetName, keyGetBinary.

• keySetBinary only marks the key as binary if the function does not return -1. I think it should set the flag regardless.

• From how keySetMeta works, I intuitively assumed keyGetMeta would just take a metaName and return the appropriate value. Ideally, these would mirror each other. But I'm sure there's a reason for this.

• ksCopy, ksRewind (maybe others) return 1 on success. Isn't it unusual to return something on success, rather than on error?

• ksAppendKey: "-1 if insertion failed, the key will be deleted then". Isn't that an implementation detail? I think it's enough to say that the function takes ownership of the provided key, and that means it handles the rest of its lifecycle no matter if it suceeds or fails. This is a bit confusing, as is.

• ksCurrentor ksNext: The note says "You must not delete or change the key".
  So change here means change where the pointer points, right, not modify the key itself?

  • I think this note could be improved to clarify, maybe "You must not delete the key or change the pointer"
  • Couldn't we specify this in the return type using Key * const instead of the current Key *? Then the note could just say "Don't delete".
  • I think ksTail should have this note too, but it doesn't.

• I'm not sure that the documentation mentions anywhere that keyset cursors can simply be incremented and decremented.

• keyDup: Like for a new key after keyNew() a subsequent ksAppend() makes a KeySet to take care of the lifecycle of the key., this should link to ksAppendKey`.

• keyAddName: "-1 if key is a null pointer or did not have a valid name before". Isn't that an invariant of a key, that it always has a valid name?

[markus2330]

As already mentioned in 2730, many methods return -1 for multiple errors. The caller cannot distinguish between errors. Examples are keySetName, keyGetBinary.

Initially, we used errno which was a pain because we had to map to error codes which were not a good fit. What we could do is to redefine the return codes so that "< 0" means error and have different negative numbers for the errors.

keyIsDirectBelow be keyIsDirectlyBelow

I think this would be the first candidate to be fixed.

keySetBinary only marks the key as binary if the function does not return -1. I think it should set the flag regardless.

Why? In general I think it is a good idea that methods do nothing on errors. Is this somewhere inconsistent?

keyGetMeta would just take a metaName and return the appropriate value

Yes, you are right, it would be more consistent. The reason behind returning the key is that this allows for checking of identity. But actually, also with a returned value one could check for identity (at least with the current implementation. If we optimize mmap it might be not the case anymore.).

One idea is that we simply return a meta key set and do not provide a separate API for meta data.

ksCopy, ksRewind (maybe others) return 1 on success. Isn't it unusual to return something on success, rather than on error?

What do you mean? They always return something?

ksAppendKey: [ownership]
ksCurrentor ksNext

I am not sure if non-C++/Rust programmers know what ownership is. Please feel free to improve the docu, it is better when we discuss this on a concrete suggestion.

I'm not sure that the documentation mentions anywhere that keyset cursors can simply be incremented and decremented.

Afaik it does not say, please add.

Isn't that an invariant of a key, that it always has a valid name?

Yes, this is poorly described. It is about keys with empty name.

You can extend the PR also with the other docu issues. Everything what you described here can be improved.

[kodebach]

keySetBinary only marks the key as binary if the function does not return -1. I think it should set the flag regardless.

Why? In general I think it is a good idea that methods do nothing on errors.

If we don't set a binary value, the key shouldn't be marked as binary. Especially since, storage plugins might rely on this metakey (via keyIsBinary).

keyGetMeta would just take a metaName and return the appropriate value

That is an oddity indeed... Changing it would, however, be a very major breaking change. We could introduce const char * keyMeta (Key * key, const char * metaName). This would mirror the API of keyString and keySetString.

One idea is that we simply return a meta key set and do not provide a separate API for meta data.

If we return an actual KeySet that would open up the possibility of metakeys having metakeys of their own. This also opens up the possibility of cycles in the references. I don't think this is a good idea...

I am not sure if non-C++/Rust programmers know what ownership is.

I agree that "ownership" is not clearly defined in C. If we use the term, we should clarify what it means to own a key.

[markus2330]

introduce const char * keyMeta (Key * key, const char * metaName). This would mirror the

Yes, good idea. Especially if it is only in libmeta (#2983) it would be nice to have it.

If we return an actual KeySet that would open up the possibility of metakeys having metakeys of their own. This also opens up the possibility of cycles in the references. I don't think this is a good idea...

see #2983

[PhilippGackstatter]

Why? In general I think it is a good idea that methods do nothing on errors. Is this somewhere inconsistent?

let binary_content: [u8; 0] = [];
let mut key = BinaryKey::new("user/test/binary").unwrap();
key.set_value(&binary_content);
println!("{}", key.is_binary());

This prints false. Is that intended? It's not really an error to set an empty array of bytes.
On the other hand, if I set binary content, such that the binary flag is set, and then set an empty string, keyIsString returns true. I suppose it can be argued that an empty string is data, while an empty array is not.

This is partly specific to Rust because of the introduction of StringKey and BinaryKey, but I had to add checks to return an empty bytes array if someone called key.binary() on a key that didn't have the binary flag set even though they had previously called key.set_value as shown above.
This is really a minor issue, just felt a bit inconsistent to me.

ksCopy, ksRewind (maybe others) return 1 on success. Isn't it unusual to return something on success, rather than on error?

What do you mean? They always return something?

Functions that can return something useful do that. But for ksCopy for instance, the success is implied if no error is returned. But it also doesn't hurt to be explicit, so forget what I said, this is also fine.

I am not sure if non-C++/Rust programmers know what ownership is. Please feel free to improve the docu, it is better when we discuss this on a concrete suggestion.

I agree that "ownership" is not clearly defined in C. If we use the term, we should clarify what it means to own a key.

Is there no typical C "language" to talk about this concept? Else I'll try to find come up with something and add it to the PR.

[markus2330]

This is really a minor issue, just felt a bit inconsistent to me.

tests/abi/testabi_key.c line 939 keySetBinary (key, NULL, 0) is successful. Maybe you use this call instead in case of size==0? It would be a Rust specific thing then. Is it currently possible to pass something like NULL?

But yes, keySetBinary (key, "", 0) (line 1040) is questionable. I am open to change that this is allowed (binary data of size 0 which is different than NULL). I do not see a use case for this, though. (Except of making Rust happy.)

Always setting binary regardless of success, however, is not a good idea.

Is there no typical C "language" to talk about this concept?

Not really, in C usually some method needs to be called to free something. So docu only describes if such a method should be called (or not).

[kodebach]

I would also like to add a few more possible API improvements:

• The internal iterator inside KeySets seems to cause more problems than it solves.
  IMO we should move to an external structure e.g.

  typedef struct _KeySetIterator {
      size_t index;
      Key * current; // in case the underlying KeySet is modified
  } KeySetIterator;

  The ensure backwards compatibility, we could embed one such iterator into KeySets, but recommend that people use an external instance in new code.
• The snippet

  KeySet * ks1 = ksDup(ks);
  KeySet * interestingPart = ksCut (ks1, key);
  ksDel (ks1);
  // .... use interestingPart ....

  or the alternative

  KeySet * interestingPart = ksCut (ks, key);
  // .... use interestingPart ....
  ksAppend (ks, interestingPart);

  are used quite frequently. We should add a counterpart to ksCut that does not modify the original KeySet. The ideal signature would be:

  // ksCopy would go better with ksCut, but it is already in use
  KeySet  * ksExtract (const KeySet * ks, const Key * parentKey);

  The const KeySet * would be possible with an external iterator, but it might even be possible with the old system.

[PhilippGackstatter]

keySetBinary (key, NULL, 0) is successful. Maybe you use this call instead in case of size==0?

Yes, this works and I changed it. Should I make it part of #2980 or a new pr, since it's not technically part of the published bindings?

But yes, keySetBinary (key, "", 0) (line 1040) is questionable. I am open to change that this is allowed (binary data of size 0 which is different than NULL). I do not see a use case for this, though. (Except of making Rust happy.)

With the above changes this is not necessary.

[markus2330]

@kodebach wrote:

I would also like to add a few more possible API improvements:

Could you please move this to two new issue, as this issue is already too broad. (ksExtract, however, would be a new feature which does not have high priority, as it is not really relevant for 1.0.)

Should I make it part of #2980 or a new pr, since it's not technically part of the published bindings?

You can add it there.

[PhilippGackstatter]

In regards to my comment about

• Couldn't we specify this in the return type using Key * const instead of the current Key *? Then the note could just say "Don't delete".

I tried changing the signature of ksNext to Key * const ksNext (KeySet *ks). If used correctly, this produces a compiler error if the caller tries to overwrite the key: error: assignment of read-only variable ‘next’. So that's nice.

Key * const next = ksNext(ks);
next = keyNew("user/otherkey", 0);

However if the caller just leaves off the const in his declaration of next, not even a warning is produced, even though ksNext has the above signature. I'm not sure why this wouldn't happen, maybe someone knows? This change only makes sense if the compiler at least warns about it.

I would propose this change for ksNext, ksCurrent, ksHead, ksTail and ksAtCursor.
In general I think it's better to encode more of the specification in the type system rather than just the documentation. Is this something I should look into more?

[kodebach]

I'm not sure why this wouldn't happen, maybe someone knows?

It's just how C works.

char x[10] = "abc";

char * a;
a = x; // changing address is valid
*a = 'c'; // changing data at address is valid

const char * b;
b = x; // changing address is valid
// *b = 'c'; !!! changing data at address is NOT valid

char * const c;
// c = x; !!! changing address is NOT valid
*c = 'c'; // changing data at address is valid

const char * const d;
// d = x; !!! changing address is valid
// *d = 'c'; !!! changing data at address is valid

a = b; // warning, because you make data at address writable
a = c; // no warning, because nothing actually changes

Copying the value of c into a doesn't change c, so we don't break the const guarantee. Changing a or the data it points to afterwards also doesn't impact c, so we still don't break any guarantees.

Copying the value of b into a also doesn't change b, but we could later on change the data that a points to (e.g. *a = 'c'), thereby breaking the const guarantee of b (since it points to the same data).

[PhilippGackstatter]

Copying the value of c into a doesn't change c

Right, so I guess * const makes no sense in any return type since pointers always get copied anyway. Thanks for the explanation!