ElektraInitiative / libelektra

Elektra serves as a universal and secure framework to access configuration settings in a global, hierarchical key database.
https://www.libelektra.org
BSD 3-Clause "New" or "Revised" License
208 stars 123 forks source link

ini plugin makes hierarchies flat #138

Closed markus2330 closed 8 years ago

markus2330 commented 10 years ago

E.g. I have:

kdb ls system/passwd

system/passwd/yacy/uid
system/passwd/zeroinst
system/passwd/zeroinst/gid

Actual outcome (excerpt): kdb export system/passwd ini

...
shell = /bin/false
uid = 114
yacy = 
gid = 265
home = /var/lib/yacy
...

Makes the hierachy flat.

Expected outcome:

[yacy]
uid=..
[zeroinst]
gid=..
fberlakovich commented 10 years ago

Currently only directories are serialised to sections. This is due to historical reasons (former treatment of keys with / in the name) and I will fix it.

fberlakovich commented 10 years ago

What is the intended behaviour? With only inner keys becoming sections, empty sections would not be possible anymore. For example, what if the kdb contained something like this

system/passwd/yacy
system/passwd/zeroinst
system/passwd/zeroinst/gid

According to your example the expected outcome would be

[yacy]
[zeroinst]
gid=..

But due to the missing subkey, yacy is no longer an inner key, but a leaf key now.

markus2330 commented 10 years ago

Hello,

You wrote:

What is the intended behaviour?

Does INI support arbitrary depth?

If not its straight forward: keyDirectBelow parentKey are sections, others are values.

With only inner keys becoming sections, empty sections would not be possible anymore. [..] But due to the missing subkey, yacy is no longer an inner key, but a leaf key now.

Thats the general topic of dir2leafvalue, see #106. One has to remember if a section corresponends to an actual key and if it has a value. This information needs to be stored in extra bits (e.g. in a subkey).

dir2leafvalue is a quite important topic: many serializations have these problems.

best regards

markus2330 commented 9 years ago

Any progress here?

I removed keyIsDir (because the name is needed for the dir-namespace). So unit tests are now broken in ini, because it wrongly relied on keyIsDir. (I temporarily commented out the comparision between files)

Instead it should rely on the key's position below parentKey.

Can you fix it for the release 0.8.11?

fberlakovich commented 9 years ago

What is the planned release date for 0.8.11?

markus2330 commented 9 years ago

Hello,

You wrote:

What is the planned release date for 0.8.11?

Asap, hopefully before the 5th.

fberlakovich commented 9 years ago

First of all: sorry for the late response. I wrote an answer 2 days ago, but obviously something went wrong.

If not its straight forward: keyDirectBelow parentKey are sections, others are values.

This is also problematic because as far as I know INI may contain key value pairs without a section. E.g.

nosectionvalue = value
[section1]
sectionvalue = value1

The value could even be empty:

nosectionvalue = 
[section1]
sectionvalue = value1

So even using the value as a differentiator is not possible. We could simply reject parsing such INI files, but then we would have yet another incomplete INI parser implementation :(. Unfortunately INI is hardly (if at all) standardized. Another possibility would be to make the behaviour configurable by allowing the user to specify section strategies (e.g. all keys below the parent key are treated as sections). However, this increases the configuration burden and would also be a fair amount of work. Which option do you like best?

markus2330 commented 9 years ago

Hello,

you wrote:

This is also problematic because as far as I know INI may contain key value pairs without a section.

The plugin "ni" solves this problem that keys without value are sections, with value are key=value, e.g.:

abc = value
abc/x = value1

would be:

abc = value
[abc]
x = value1

w/o abc = value would be without first line.

The value could even be empty:

So even using the value as a differentiator is not possible. We could simply reject parsing such INI files, but then we would have yet another incomplete INI parser implementation :(. Unfortunately INI is hardly (if at all) standardized. Another possibility would be to make the behaviour configurable by allowing the user to specify section strategies (e.g. all keys below the parent key are treated as sections). However, this increases the configuration burden and would also be a fair amount of work. Which option do you like best?

In Elektra empty value != null (without) value. So it should work nicely. See keyValue() docu

Or you can check presence/absence of keys. Shouldn't that be enough already? Just do not generate the key abc if there is a section but not a key with (empty) value.

fberlakovich commented 9 years ago

@markus2330 Is there currently any way to unescape a key name? My problem is that currently an ini key test/key will produce an Elektra key like user/ini/test\/key. However, when writing the key back it will result in an ini key test\/key which is obviously not the same.

I would even more prefer to already have a way to set the key name unescaped. This would allow the ini file

[/settings/NRPE/server]
allow arguments = true

to result in the very intuitive Elektra keyset

user/settings/NRPE/server
user/settings/NRPE/server/allow arguments

(example taken from http://docs.nsclient.org/tutorial/core/index.html)

markus2330 commented 9 years ago

Hello,

you wrote:

@markus2330 Is there currently any way to unescape a key name? My problem is that currently an ini key test/key will produce an Elektra key like user/ini/test\/key. However, when writing the key back it will result in an ini key test\/key which is obviously not the same.

keyAddBaseName is to add only one part and thus will escape slashes. keyAddName (to be added in the API with the next release, but to be renamed to keyChangeName) changes a key-name as given, without escaping any part. keySetName also does not escape the name.

If you find the documentation confusing, please report where which information should be added.

best regards

markus2330 commented 9 years ago

Thank you! There was a lot of progress on this issue. Unfortunately, the import/export shell tests (e.g. tests/shell/check_export.sh) still do not support ini.

In the export test case an ini file is generated which has two times the same key.

markus2330 commented 9 years ago

Problem still persists. E.g. for structural exporting ini is basically useless:

$ kdb export system/info/constants ini
cmake = ON
cmake = ON
cmake = ON
cmake = /usr
cmake = OFF
cmake = OFF
cmake = OFF
...
fberlakovich commented 9 years ago

This plugin is getting nasty. Could you provie the file you have mounted at system/info/constants?

markus2330 commented 9 years ago

You can mount it with the script "mount-info", but here a different export with ni:

$ kdb export system/info/constants ni
version/version/KDB_VERSION = 0.8.11
compiler = Flags defined for compilers as defined in ElektraCompiling.cmake
cmake/TARGET_CMAKE_FOLDER = share/cmake-2.8/Modules
compiler/id = GNU
cmake = All cmake variables as defined in the file cmake/ElektraCache.cmake
cmake/ENABLE_CXX11 = OFF
cmake/GTEST_ROOT = 
macros/KDB_DIR_MODE = 0100
version/SO_VERSION = 4
cmake/ELEKTRA_VERBOSE_BUILD = OFF
cmake/BUILD_STATIC = ON
cmake/TOOLS = kdb\;gen\;race\;qt-gui
cmake/TARGET_TEST_DATA_FOLDER = share/libelektra-test/test-data
cmake/KDB_DB_USER = .config
cmake/TARGET_INCLUDE_FOLDER = elektra

ni scrambles the order, so ini would be a nice gain ;)

markus2330 commented 9 years ago

Btw. the release is most likely to happen today and this issue is not release-critical. I will merge it if its a small change though.

fberlakovich commented 9 years ago

You can mount it with the script "mount-info", but here a different export with ni:

Thank you, I will have a look at it!

ni scrambles the order, so ini would be a nice gain ;)

No matter how nasty it is, I already invested so much work in ini, it would be a shame not to fix it :).

fberlakovich commented 9 years ago

Btw. the release is most likely to happen today and this issue is not release-critical. I will merge it if its a small change though.

The plugin is fairly complex already (especially this issue), so I am afraid I won't make it today.

fberlakovich commented 9 years ago

As discussed, here is a more detailed explanation of the issue:

Let me first explain what we would expect from the plugin just to make sure we have the same understanding of its (expected) behavior. A call like kdb export system/info/constants ini should produce an ini file like this:

cmake = All cmake variables as defined in the file cmake/ElektraCache.cmake
cmake/BINDINGS = cpp
cmake/BUILD_FULL = ON
...

assuming that a call like kdb ls system/info/constants looks like this:

system/info/constants
system/info/constants/cmake
system/info/constants/cmake/BINDINGS
system/info/constants/cmake/BUILD_FULL
...

(values don't really matter for this issue). No sections are produced because none of the keys below system/info/constants qualifies as a section key and the autosections option is not enabled.

However, the actual output looks like this:

cmake = All cmake variables as defined in the file cmake/ElektraCache.cmake
cmake = cpp
cmake = ON

Now that the issue is fully described let me explain why it happens:

The bug is caused by the call of elektraUnescapeKeyName in the function getIniName. The function is responsible for constructing a correct ini name out of the supplied key and its parent key. This involves finding the correct section of the key (if any) and afterwards constructing the key name based on that information. The function calls elektraUnescapeKeyName because usually an ini name like cmake/BINDINGS would result in a key system/info/constants/cmake\\BINDINGS. Unescaping this key and applying the transformations done in getIniName would correctly result in the name cmake/BINDINGS. However, if the function elektraUnescapeKeyName is applied to a string that doesn't contain any escaping it replaces all / by \0. So applying it to cmake/BINDINGS (as it is done during the export) results in cmake\0BINDINGS which in turn leads to the corruped output.

In order to fix this issue the ini plugin needs a way to detect whether unescaping needs to be done. The only possible way I could think of up to now is marking escaped keys with a special meta key during the GET phase. However, this would make the ini plugin stateful which I don't really like.