doc: add decision docs for decision meeting 15.07.2021

doc/decisions/README.md

@@ -50,11 +50,17 @@ section here.
 - [Ensure](ensure.md) (@kodebach)
 - [Capabilities](capabilities.md) (@markus2330)
 - [Error Semantics](error_semantics.md) (API)
+- [Error Handling](error_handling.md)
+- [Remove elektraMalloc et al.](remove_elektra_malloc.md)
+- [Functions copying into buffer](functions_with_buffers.md)
 
 ## Decided
 
 - [Array for Warnings](warning_array.md)
 - [Array](array.md)
+- [Iterators](iterators.md)
+- [keyString() return value](key_string_return_value.md)
+- [Reference Counting](reference_counting.md)
 
 ## In Discussion
 

doc/decisions/error_handling.md

@@ -0,0 +1,39 @@
+# Error Handling
+
+## Problem
+
+Currently, a lot of functions return the error value, regardless of error. These poses some problems, for instance authors of bindings, where it is hard to tell which error exactly occurred. A big improvement to this would be returning different error codes for different types of errors, in order to be able to provide better information in cases of errors. Some functions also make use of an `errorKey`.
+
+While this seems like a decent approach, to provide additional informations in cases of error, it is currently very badly documented. It is not really transparent for the users of the function, what kind of information is included in the `errorKey` and even which messages could possibly be returned.
+
+## Constraints
+
+We also have to consider that many parts of the API very constrained in what values they can use for error codes. If a function returns a pointer the only possible error value by definition is NULL.
+
+## Assumptions
+
+## Considered Alternatives
+
+- Change return types for many functions across the API
+
+## Decision
+
+- Update documentation in `doc/dev/error-*` and link to them in the documentation
+  for the module `kdb`
+- Add second channel for getting information about errors
+- Return error codes directly from functions where failures are expected, e.g. `kdbGet`, `keySetName`
+- Harmonize return values from all functions and move error reporting to second channel
+
+## Rationale
+
+This simplifies and unifies error reporting across all functions. It also introduces
+a default way for the developers to handle error reporting. Developers of
+bindings for Elektra can rely on the existing error reporting feature without
+having to introduce custom error messages, that can vary wildly between different
+bindings.
+
+## Implications
+
+## Related Decisions
+
+## Notes

doc/decisions/functions_with_buffers.md

@@ -0,0 +1,43 @@
+# Functions copying into buffers
+
+## Problem
+
+Currently the way functions like keyGetName() work is by passing a buffer with
+a maxSize and if the buffer is large enough, the value gets copied into the
+buffer. This leads to the user having to write a lot of surrounding boilerplate
+code, checking for the size of every value / name they want to copy into a buffer.
+
+## Constraints
+
+## Assumptions
+
+## Considered Alternatives
+
+## Decision
+
+- Remove Functions:
+  - keyGetName()
+  - keyGetUnescapedName()
+  - keyGetBaseName()
+  - keyGetString()
+  - keyGetBinary()
+- add documentation in API docu about life-time and add in release notes that you should use strncpy() / memcpy() instead:
+
+```c
+// str values
+strncpy(..., keyName (k), ...)
+// binary values
+memcpy(..., keyValue (k), ...)
+```
+
+## Rationale
+
+The functions clutter the API and try to replace existing builtin functionality
+for little to no gain. This makes the API leaner while also retaining its
+functionality.
+
+## Implications
+
+## Related Decisions
+
+## Notes

doc/decisions/iterators.md

@@ -0,0 +1,55 @@
+# Iterators
+
+## Problem
+
+The internal iterator inside KeySets seems to cause more problems than it solves.
+
+## Constraints
+
+We could embed one such iterator into KeySets, but recommend that people use an
+external instance (int) in new code.
+
+## Assumptions
+
+@raphi011 made benchmarks showing that external and internal iterators have
+about the same speed. So it should be a safe choice to not provide the internal
+iterators for 1.0 and only the external instead.
+
+## Considered Alternatives
+
+## Decision
+
+- remove all functions related to the internal iterator:
+  - ksRewind
+  - ksNext
+  - ksCurrent
+  - ksGetCursor
+  - ksSetCursor
+  - ksHead
+  - ksTail
+  - keyRewindMeta
+  - keyNextMeta
+  - keyCurrentMeta
+- change `ksAtCursor` to `ksAt`
+- add implementation / documentation / tests for the external iterator
+- start using external iterators in new code
+- remove docu about internal cursor from all functions.
+
+## Rationale
+
+- The only function that returns a `cursor_t` is `ksGetCursor`.
+  Its documentation is completely broken:
+- `ksRewind` and `keyNextMeta` cannot be used on the same variable (`ks`).
+- The documentation (not just for the above function, but all over the `ks*`
+  functions) also contains lots of warnings (use only cursor from same keyset,
+  may be invalid, may become invalid, etc.) that make it seem like there is
+  something special about these cursors, when in fact they are simply indicies.
+  There is no information (at least that I could find), how cursors can be
+  modified. Most of the time it even seems like modifying cursors would be a bad
+  idea.
+
+## Implications
+
+## Related Decisions
+
+## Notes

doc/decisions/key_string_return_value.md

@@ -0,0 +1,39 @@
+# keyString() return value
+
+## Problem
+
+When using keyString() on empty / binary values the return values are the literal strings (null) / (binary). This seems very awkward and unintuitive from a user's perspective.
+
+## Constraints
+
+Because the return value of the function is const char\*, there are not so many possibilities for a change to this behavior, 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 behavior would be:
+
+## Assumptions
+
+On one hand always returning a string allows things like `strlen(keyString(k)) == 0` to 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`.
+
+## Considered Alternatives
+
+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.
+
+## Decision
+
+- `key == NULL` return 0, error code via second channel
+- `key->value == NULL` return 0, error code via second channel
+- `key == <binary>` return 0, error code via second channel
+- everything else as is
+
+## Rationale
+
+- 0 seems like the most intuitive value to return in case of an error, although
+  this introduces the possibility of segfaults for users of the library. printf
+  doesn't cause segfaults anymore, which improves this problem a lot.
+- With the introduction of a second channel for reporting errors, users can check the
+  error messages in case of segfaults - which alleviates this issue. The first
+  thing in case of an error should be checking the error message.
+
+## Implications
+
+## Related Decisions
+
+## Notes

doc/decisions/reference_counting.md

@@ -0,0 +1,39 @@
+# Reference Counting
+
+## Problem
+
+- locking is not reset when ref counting again gets 0 (adding to keyset and
+  pop again) #2202
+- C++ API for KeySet and Key has unexpected differences: also use ref counting
+  for KeySets (also suggested in #1332)
+
+## Constraints
+
+## Assumptions
+
+## Considered Alternatives
+
+- make ref counting thread safe (probably useful for JNI)
+- start with 1 for reference counting and let keyDecRef do keyDel
+
+## Decision
+
+- add second reference counter to Key
+- One counter is for locking references, the other one for general references. A locking reference automatically locks/unlocks the keyname.
+- introduce reference counter for KeySets (for external keyset references)
+- `keyBorrow` returns an error in case of reference counter overflow
+- use fixed sized types for reference counters
+- increment/decrement references before/after passing instances to plugins
+
+## Rationale
+
+- Adding a second reference counter to Key and reducing the size of both significantly (`size_t` to `uint16_t`)
+  actually saves memory (32 vs 64bit on 64-bit machines) compared to the previous solution.
+- The added complexity of maintaining two reference counters is worth the
+  tradeoff for the gained functionality.
+
+## Implications
+
+## Related Decisions
+
+## Notes

doc/decisions/remove_elektra_malloc.md

@@ -0,0 +1,60 @@
+# Remove elektraMalloc() et al.
+
+## Problem
+
+A big problem with having elektraMalloc is, that it makes it impossible to call
+libc functions that allocate data with malloc (e.g. strndup) or others that
+expect a pointer that can be passed to realloc. You might even do that by
+accident and never notice the problem, until someone replaces elektraMalloc
+and then you could get some pretty bad memory bugs.
+
+Functions considered for removal:
+
+- `elektraMalloc`
+- `elektraCalloc`
+- `elektraFree`
+- `elektraStrDup`
+- `elektraStrLen`
+
+## Constraints
+
+Proprietary apps are sometimes delivered together with a libc (either static or
+a startup script makes sure their own libs are found). If they would use
+Elektra, and the person wants the app to use the global Elektra it is a valid
+use case for the dedicated functions.
+
+## Assumptions
+
+The only safe way to keep elektraMalloc et al. is to define that they will
+always call their libc counterpart (malloc et al.) and their purpose is simply
+to add assertions. Then we can also make them private, because you can just
+call free to free any pointer returned by Elektra. Ideally, I'd remove the
+functions, but it seems unlikely this will be accepted.
+
+## Considered Alternatives
+
+- Completely replace `elektraMalloc` / `elektraCalloc` with simple calls to malloc
+
+## Decision
+
+- keep current state with the custom functions
+- make it optional for plugin developers to use c builtins or the custom Elektra
+  functions
+- Fix places where non-Elektra functions get used
+- Remove everything except `elektraFree` from public API
+- Remove all functions that don't actually involve any memory allocations (e.g. `elektraStrLen`, `elektraStrCmp`)
+- Add `elektraStrNDup` and other `stdlib` equivalents that do memory allocation.
+
+## Rationale
+
+While the current state might cause some problems with Compiler optimizations
+and discoverability, those issues probably are only very minor in practice.
+Removing all functions poses a considerable amount of work, which would have to
+be undone in case we need those functions one day anyway. The current downsides
+don't justify such a big procedure.
+
+## Implications
+
+## Related Decisions
+
+## Notes

doc/news/_preparation_next_release.md

@@ -159,6 +159,7 @@ _(Michael Tucek)_
 - Add debugging tutorial. _(Tobias Schubert @qwepoizt)_
 - Improve wording and formatting of DESIGN.md _(@lawli3t)_
 - Correct various typing-, spelling- and grammar-errors in the .md-files in the directory doc and its subdirectories. _(Florian Lindner @flo91)_
+- Added documentation for decision meeting from 15.07.2021 _(@lawli3t)_
 - <<TODO>>
 - explained in the docker test tutorial how to run the container with podman instead of docker. _(@muskater)_
 - Add a new example on how to use keyCopy. _(@muskater)_