Decisions

[markus2330]

Finally, the outcomes from the last meeting. Not perfect yet but we can continue our discussions.

Basics

These points need to be fulfilled for every PR:

• Short descriptions of your changes are in the release notes
  (added as entry in doc/news/_preparation_next_release.md which
  contains _(my name)_)
  Please always add something to the release notes.
• Details of what you changed are in commit messages
  (first line should have module: short statement syntax)
• References to issues, e.g. close #X, are in the commit messages.
• The buildservers are happy.
• The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

• I added unit tests for my code
• I fully described what my PR does in the documentation
  (not in the PR description)
• I fixed all affected documentation
• I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
• I updated all meta data (e.g. README.md of plugins and METADATA.ini)
• I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

• Documentation is introductory, concise, good to read and describes everything what the PR does
• Examples are well chosen and understandable
• Code is conforming to our Coding Guidelines
• APIs are conforming to our Design Guidelines
• Code is consistent to our Design Decisions

Labels

If you are already Elektra developer:

• Add the "work in progress" label if you do not want the PR to be reviewed yet.
• Add the "ready to merge" label if the basics are fulfilled and you also
  say that everything is ready to be merged.

[markus2330]

@mpranj @kodebach which are the decisions where you cannot agree in the current form (as in this PR) or where you think it would be a mistake to decide it without further considerations/discussions?

[kodebach]

It should be clear from my comments, which decision still need work IMO. But just in case:

• arbitrary_metadata needs some clarifications regarding things like kdb set-meta user:/key mymeta 5.
• boolean is basically done, but could be worded a bit more clearly.
• ensure I disagree with the entire decision, except that it should only work for global plugins
• escaped_name I agree with the basis, but I don't think it can be decided as is. What can be decided is that the escaped name will no longer be stored and should used only when necessary. The exact API (what will be renamed and, how keyNew will work etc.) is still very much up in the air IMO.

[kodebach]

Not 100% sure this is the best possible API, but yes something like this is what I thought of.

The proposal is a bit inconsistent. AFAICT elektraIoSetBinding is a separate function to avoid having a Key whose value is a ElektraIoInterface *. But then there is elektraGOptsGetContract which needs to store char **, which has the same problems.

So either, both cases use a separate setter function called after kdbOpen that directly manipulates the relevant Plugin struct, or they both just use a *GetContract function that creates binary keys.

Personally, I think a separate getter makes more sense. The whole point of having a contract KeySet is that the API/ABI doesn't have to change if additional config options are added. A elektraNotificationGetContract function would have to change, if e.g. notifications get some new config option.

[markus2330]

Not 100% sure this is the best possible API,

Also not yet sure, here is an improvement: 581c6bf

[kodebach]

Yes, the new API looks a lot better. However, I would prefer, if either kdbOpen took ownership of the KeySet * contract or at least copied what is needed, so that ksDel (contract) can be called right after kdbOpen. Otherwise there is too much potential for memory leaks IMO.

[markus2330]

Yes, of course it will copy, so ksDel afterwards is safe: a6429dc

Taking ownership can easily lead to misunderstandings, especially because KeySet is different to Key (which is ref-counted, so the ownership can be shared even if "taken" like in ksNew). In elektraPluginOpen we have such an API and I actually think we should change it there (if we decide to make this API public).

[kodebach]

I like were the decision on metadata is going, but I think we might need another more in depth discussion and should maybe move it to a separate PR.

What I got from your proposal are these key points ("store" means "write into storage file"):

• metadata that is used as specification (e.g. check/ipaddr, or execute/set) are stored only in spec:/ files
• other namespaces do not store any metadata, but storage plugins can infer some metadata from files (e.g. type, comment, array)
• the spec plugin does a lot of heavy lifting

AFAICT it we are basically going in the direction of separating the two uses of metadata: specification, and inferred meta-information.

IMO the best solution would be, if plugins like network and shell would take their specifications (e.g. check/ipaddr and execute/set) directly from the spec:/ keys, without the spec plugin having to copy everything to all namespaces. This would however be a massive change, so it is probably out of scope. It is also unclear, how the spec:/ keys would even be passed to non-global plugins.

The next best thing would be, if the spec plugin copied the metadata into meta-keys with namespace metaspec:/. So a Key can have meta:/ and metaspec:/ metadata. The meta:/ keys are the one inferred from the storage file or from the specification, and the metaspec:/ keys are the actual specification. This would still require updating many plugins, but it would be a quite simple change (just adding the metaspec:/ prefix shouldn't be too hard). This also avoids the dual meaning of e.g. array; metaspec:/array=#0 is "default size 1" and meta:/array=#0 is "actual size is 1". It would also make the spec plugin a bit simpler. For example spec doesn't need to handle merging type from spec:/ keys and other namespaces. [1] Instead the type plugin could handle that. For example:

{ "mykey": 4 }

produces a key mykey=4 with metadata type=long_long. If there is also a specification where mykey has type=int, this wouldn't be a problem. The type plugin would just do the normal validation between metaspec:/type=int and the value 4 and then do an additional check that a whether meta:/type can be assigned to metaspec:/type (e.g. int can be assigned to long_long).

Assuming that applications don't need direct access to the metaspec:/ metadata, the metaspec:/ metadata could also be discarded at the end of kdbGet and regenerated at the start of kdbSet to avoid any inconsistencies between the specification in the spec:/ namespace and the metaspec:/ metadata.

---

[1] spec would not only copy to metaspec:/ it also still needs to create some meta:/ metadata (e.g. type and array) in some cases.

[markus2330]

ensure I disagree with the entire decision, except that it should only work for global plugins

Ok, as you saw I reworked it completely. 581c6bf

escaped_name I agree with the basis, but I don't think it can be decided as is. What can be decided is that the escaped name will no longer be stored and should used only when necessary. The exact API (what will be renamed and, how keyNew will work etc.) is still very much up in the air IMO.

Yes, true, I renamed the sections so that it is clear that there is still a lot to be decided about the API. 788d273

[mpranj]

Thank you for updating the decisions from the meeting!

[markus2330]

Thank you for all your great and valuable input at the meeting 💖

[mpranj]

Looks great, I only found some typos.

[mpranj]

Thank you so much!