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
206 stars 123 forks source link

Setting of values not working for cascading names with highlevel API #4337

Closed flo91 closed 1 year ago

flo91 commented 2 years ago

Steps to Reproduce the Problem

While working on CM-H2, I noticed that the values are correctly read from the KDB, but setting of values fails silently, so everything seems to run smoothly and no ElektraError is set, but the changes are not written to the KDB. I used the highlevel-API with code generation for the homework.

To further investigate the issue, I tried a simplified version of the example which is available at https://github.com/ElektraInitiative/libelektra/tree/master/examples/highlevel. I used the cmake variant.

The exact code of my sample application is:

#include <stdio.h>
#include <stdlib.h>
#include <elektra.h>

static void onFatalError (ElektraError * error)
{
    fprintf (stderr, "ERROR: %s\n", elektraErrorDescription (error));
    elektraErrorReset (&error);
    exit (EXIT_FAILURE);
}

int main (int argc, char ** argv)
{
    ElektraError * error = NULL;
    Elektra * elektra = elektraOpen ("/sw/example/highlevel/#0/current", NULL, NULL, &error);
    if (!elektra)
    {
        fprintf (stderr, "An error occured while opening Elektra: %s", elektraErrorDescription (error));
        elektraErrorReset (&error);
        return EXIT_FAILURE;
    }

    elektraFatalErrorHandler (elektra, onFatalError);

    printf ("Value of mystring: %s\n", elektraGetString (elektra, "mystring"));
    elektraSetString (elektra, "mystring", "set from app", &error);
    if (error)
        fprintf (stderr, "An error occurred while trying to set mystring");

    printf ("Value of myint: %d\n",  elektraGetLong (elektra, "myint"));
    elektraSetLong (elektra, "myint", 42, &error);
    if (error)
        fprintf (stderr, "An error occurred while trying to set myint");

    printf ("Value of mydouble: %f\n", elektraGetDouble (elektra, "mydouble"));

    elektraClose (elektra);
    return EXIT_SUCCESS;
}

I used the specification from the example: https://github.com/ElektraInitiative/libelektra/blob/master/examples/highlevel/spec.ini

In the folder /home/flo/tu/cm/elektra-0.9.9/examples/highlevel I executed the following commands:

sudo kdb mount spec.ini spec:/sw/example/highlevel/#0/current ni
sudo kdb import spec:/sw/example/highlevel/#0/current ni "$PWD/spec.ini"
sudo kdb spec-mount '/sw/example/highlevel/#0/current' ni

Then I tried to query some values and got the default values as defined by the specification:

kdb get /sw/example/highlevel/#0/current/mystring
#>
kdb get /sw/example/highlevel/#0/current/myint
#> 0

Following keys were available at that time: (standard system keys omitted)

kdb ls /
#> spec:/sw/example/highlevel/#0/current
#> spec:/sw/example/highlevel/#0/current/mydouble
#> spec:/sw/example/highlevel/#0/current/myfloatarray/#
#> spec:/sw/example/highlevel/#0/current/myint
#> spec:/sw/example/highlevel/#0/current/mystring
#> spec:/sw/example/highlevel/#0/current/print
#> default:/sw/example/highlevel/#0/current/mydouble
#> default:/sw/example/highlevel/#0/current/myint
#> default:/sw/example/highlevel/#0/current/mystring
#> default:/sw/example/highlevel/#0/current/print

Now I ran the application two times. The expected behaviour would be that the output of the second run should reflect the changes made by calling elektraSetString and elektraSetLong. As you can see, no changes were written to the KDB.

./application 
#> Value of mystring: 
#> Value of myint: 0
#> Value of mydouble: 0.000000

./application
#> Value of mystring: 
#> Value of myint: 0
#> Value of mydouble: 0.000000

Now I tried to manually set a new value for mystring using a cascading name:

kdb set /sw/example/highlevel/#0/current/mystring test
#> Set string to "test"
#> Using name default:/sw/example/highlevel/#0/current/mystring

kdb get /sw/example/highlevel/#0/current/mystring
#>
kdb get default:/sw/example/highlevel/#0/current/mystring
#>

As you can see, the changed value was not returned by the kdb get with the same cascading name or default-namespace.

In the next step, I executed the set-command explicitly for the user-namespace. Now the value was saved and returned by kdb get.

kdb set user:/sw/example/highlevel/#0/current/mystring test
#> Create a new key user:/sw/example/highlevel/#0/current/mystring with string "test"
kdb get /sw/example/highlevel/#0/current/mystring
#> test

But when running the application, the value was still not changed:

./application 
#> Value of mystring: test
#> Value of myint: 0
#> Value of mydouble: 0.000000

./application
#> Value of mystring: test
#> Value of myint: 0
#> Value of mydouble: 0.000000

A kdb set with a cascading name was now working, because the user-namespace was used instead of the default-namespace:

kdb set /sw/example/highlevel/#0/current/mystring test2
#> Set string to "test2"
#> Using name user:/sw/example/highlevel/#0/current/mystring

kdb get /sw/example/highlevel/#0/current/mystring
#> test2

But the application was still not working:

./application
#> Value of mystring: test2
#> Value of myint: 0
#> Value of mydouble: 0.000000

./application
#> Value of mystring: test2
#> Value of myint: 0
#> Value of mydouble: 0.000000

The "solution" I found was to add the user-namespace in the elektraOpen()-call:

Elektra * elektra = elektraOpen ("user:/sw/example/highlevel/#0/current", NULL, NULL, &error);
./application
# RET: 1
#> Value of mystring: test2
# STDERR: ERROR: The key 'user:/sw/example/highlevel/#0/current/myint' could not be found.

./application
# RET: 1
#> Value of mystring: set from app
# STDERR: ERROR: The key 'user:/sw/example/highlevel/#0/current/myint' could not be found.

I finally added the missing keys to the user-namespace, so that the applications runs successfully.

kdb set user:/sw/example/highlevel/#0/current/myint 2
#> Create a new key user:/sw/example/highlevel/#0/current/myint with string "2"

kdb set user:/sw/example/highlevel/#0/current/mydouble 0.2
#> Create a new key user:/sw/example/highlevel/#0/current/mydouble with string "0.2"

./application
#> Value of mystring: set from app
#> Value of myint: 2
#> Value of mydouble: 0.200000

./application
#> Value of mystring: set from app
#> Value of myint: 42
#> Value of mydouble: 0.200000

But as you can see, now only the values that are explicitly available for the specified namespace are considered, so no cascading lookup was used any more which can lead to various problems... (e.g. no default values used)

Expected Result

The calls to elektraSetString(...) and elektraSetLong(...) should change values in the KDB.

Actual Result

The values were not changed, unless giving a namespace in the call elektraOpen(...)

System Information

kodebach commented 2 years ago

Thanks for reporting the issue and providing just a detailed description.

Seems like elektraSetRawString is simply broken.

https://github.com/ElektraInitiative/libelektra/blob/f2b9d555a2de6da944781bd9edb4a2a877cd3c35/src/libs/highlevel/elektra_value.c#L160-L168

This just calls elektraSaveKey with a cascading key. So AFAICT this just adds a cascading key to the keyset within the Elektra handle and then calls kdbSet. Interesting that this went unnoticed for so long. It simply cannot work like this. We need a ksLookup (or elektraFindKey) somewhere.

However, as you saw with kdb set. This still won't be enough, when there is only a default value. We'd need to choose a namespace. How this would best be achieved, I don't know.

flo91 commented 2 years ago

Thank you for the quick response and the good explanation!

Hm, that seems like a serious issue. I guess for the CM-homework it's best to explicitely give the user-namespace in the parameter of elektraOpen(...) and provide a script that prepares the KDB so that the application can work with it.

About choosing a namespace for new keys, I think the most natural behaviour would be to use the user-namespace for processes started by a user and the system-namespace if the process was started by root.

If the key was already defined in the dir-namespace, it should be written there. On the other hand, if a process was started by a user and a value is saved, but a key only exists in the system-namespace, I would create a new key in the user-namespace.

But that's just a short first impression, there are for sure some problems and special cases that we have to consider.

markus2330 commented 2 years ago

Also a big thanks from me! Afaik the HL API was only used for getting values up-to-now. For the deadline today, we will simply ignore the setting keys in your submission (as StarHotel does not include setting keys at all, it would have been a bonus task anyway).

Parts of the report (about cascading kdb set) are already covered in #4028.

The correct namespace to use for writing is application dependent. E.g. in KDE new config values are written to user:, which is probably a sane default for the HL API. But as it is not the right choice for every application, we probably should provide a elektraSetCascadingWriteNamespace to modify the namespace. (Adding a parameter to every elektraSet call is maybe not so ideal, as it already has 4 parameters).

@kodebach do you think you can fix this in a reasonable timeframe so that @flo91 can use use it for the correction deadline?

kodebach commented 2 years ago

we probably should provide a elektraSetCascadingWriteNamespace to modify the namespace.

I also thought about something like that. However, I'd simply call it elektraDefaultNamespace:

// only ELEKTRA_NS_USER, ELEKTRA_NS_DIR and ELEKTRA_NS_SYSTEM are accepted
void elektraDefaultNamespace (Elektra * elektra, ElektraNamespace defaultNamespace)

As the name suggests, the namespace is used as a default, if a) currently no key exists at all or b) only a default:/ or proc:/ key exists. If a dir:/, user:/ or system:/ key exists, we use that one (in this order, like always).

This should cover the common use case. If the need arises, we could also add:

// only ELEKTRA_NS_USER, ELEKTRA_NS_DIR and ELEKTRA_NS_SYSTEM are accepted
void elektraWriteNamespace (Elektra * elektra, ElektraNamespace writeNamespace)

This namespace would be used for all write operations no matter what keys do or don't exist already.

Note: There is no "set" in the function names, since elektraFatalErrorHandler also doesn't have a "set" in the name

kodebach do you think you can fix this in a reasonable timeframe so that flo91 can use use it for the correction deadline?

If it is indeed as simple as I currently believe than yes, that should be possible.

markus2330 commented 2 years ago

Even if it is not urgent right now, we should not forget to fix this.

kodebach commented 2 years ago

@markus2330 Probably a good FLOSS issue. The solution is basically described above, but there are still some details to work out for the implementor.

flo91 commented 2 years ago

@markus2330 As @kodebach already mentioned, this issue seems good for the FLOSS course.

github-actions[bot] commented 1 year ago

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

github-actions[bot] commented 1 year ago

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart: