close #4431: kdb cli rewrite

[hannes99]

I'm open to feedback on the general structure of the new CLI, the earlier the better.

Issue: #4431

Basics

• 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. If not, fix in this order:
  • add a line in doc/news/_preparation_next_release.md
  • reformat the code with scripts/dev/reformat-all
  • make all unit tests pass
  • fix all memleaks
• The PR is rebased with current master.

Checklist

• 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 reuse syntax

Review

• 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

• 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 no further pushes are planned by you.

[kodebach]

I'm not sure about the struct command approach. I get that you wanted to keep the commands separate, but this is quite the leaky abstraction. It still requires changes in main.c to add a command. I also have some concrete questions:

1. What would checkArgs be used for?
2. How would nested commands work? e.g. kdb meta get, kdb meta set where get/set are sub-commands of meta.

---

The linear search for the correct sub-command is really not how the sub-command parsing was meant to work. The reason, the command keys are set to the base name of the sub-command that was invoked is so that you can use keyAddBaseName. This may not be very well documented and probably could use an actual example somewhere, but the idea was to do something like this:

   Key * lookupKey = keyNew (CLI_BASE_KEY, KEY_END);
   Key * commandKey = ksLookup (options, commandKey, 0);
   while (keyGetValueSize (commandKey) > 1) // empty string has size 1, because of zero-terminator
   {
       keyAddBaseName (lookupKey, keyString (commandKey));
       // TODO: process intermediate options of command defined by current commandKey
       commandKey = ksLookup (options, commandKey, 0);
   }
   // TODO: execute the command defined by commandKey

This approach naturally handles nested commands, it has a well defined and obvious interpretation for things like kdb -v get -v /foo (-v option of kdb and separate -v option of get used, options of kdb will be processed first), and it should also be faster when there are large numbers of sub-commands.

With this in mind, I think the commands should be defined by two KeySets with similar structure.

// The first KeySet defines the spec passed to elektraGetOpts
KeySet * commandsSpec = ksNew (1,
     keyNew ("spec:" CLI_BASE_KEY "/version", KEY_META, "description", "Print version info.", KEY_META, "opt", "V", KEY_META, "opt/long", "version", KEY_META, "opt/arg", "none", KEY_END),
     keyNew ("spec:" CLI_BASE_KEY "/get", KEY_META, "description", "Get the value of an individual key.", KEY_META, "command", "get", KEY_END),
     keyNew ("spec:" CLI_BASE_KEY "/get/all", KEY_META, "description", "Consider all of the keys", KEY_META, "opt", "a", KEY_META, "opt/long", "all", KEY_META, "opt/arg", "none", KEY_END),
    KS_END);

// The second KeySet defines the functions for the commands that could be executed
KeySet * commands = ksNew (1,
      keyNew (CLI_BASE_KEY "/get/exec", KEY_FUNC, execGet, KEY_END),
      keyNew (CLI_BASE_KEY "/get/checkargs", KEY_FUNC, checkGetArgs, KEY_END),
      KS_END);

Sidenote: @markus2330 The second KeySet is another example, of where using cascading keys for data is useful, i.e. a new namespace as suggested in #4415 would be needed, if this use of cascading is forbidden. Alternatively, one of the existing namespaces would have to be chosen arbitrarily.

Then you can do something like this, whenever you need a function specific to a command:

// Note: error handling omitted
typedef int (*execFunc) (KeySet * options, Key * errorKey);

// assume commandKey is currently e.g. `CLI_BASE_KEY "/get"`
keyAddBaseName (commandKey, "exec");
Key * execKey = ksLookup (commands, commandKey, 0);
execFunc fnExec = *(execFunc *) keyValue (execKey);
keySetBaseName (commandKey, NULL);

// Now we can call the function
fnExec (options, errorKey);

You could also store something like struct command in the key values of the KeySet * commands similar to what I did with the new KeySet * backends in struct _KDB on the new-backend branch:

libelektra/src/libs/elektra/kdb.c

Lines 385 to 398 in 5f0895b

	static void addMountpoint (KeySet * backends, Key * mountpoint, Plugin * backend, KeySet * plugins, KeySet * definition)
	{
	BackendData backendData = {
	.backend = backend,
	.keys = ksNew (0, KS_END),
	.plugins = plugins,
	.definition = definition,
	.getSize = 0,
	.initialized = false,
	.keyNeedsSync = false,
	};
	keySetBinary (mountpoint, &backendData, sizeof (backendData));
	ksAppendKey (backends, mountpoint);
	}

Then the whole pointer casting and function accessing stuff might seem a bit more natural

libelektra/src/libs/elektra/kdb.c

Lines 1436 to 1448 in 5f0895b

	BackendData * backendData = (BackendData *) keyValue (backendKey);
	// check if get function exists
	// TODO (kodebach): move check into kdbOpen
	kdbGetPtr getFn = backendData->backend->kdbGet;
	if (getFn == NULL)
	{
	ELEKTRA_ADD_INTERFACE_WARNINGF (
	parentKey, "The mountpoint '%s' defined a plugin ('%s') without a kdbGet function as a backend.",
	keyName (backendKey), backendData->backend->name);
	success = false;
	continue;
	}

Another advantage is that when you add a new field to the struct, you'll have to explicitly define a value for every command. With the "functions pointers/values directly in key values" approach from above, you'd always have to handle the case of a missing key. And of course you'll only have to do one key name change & lookup per command instead of one for every function you need.

---

I do have some other more specific notes on parts of the code, but I'll hold off on a full review, until we've decided on a general structure.

[hannes99]

Thanks for the feedback @kodebach !

For your questions:

1. I felt like it would make sense to be able to validate given data, and I thought it should be as straightforward as possible when adding a new command. So a function for it that is called by the CLI when needed seemed reasonable, however, it may very well be that in the context of this CLI it does not make sense.
2. Here the idea was that each subcommand takes care of all its subcommand, so main just does addMetaSpec and execMeta and the meta command takes care of the spec and execution of its subcommand. So every command only ever knows its immediate subcommand. For adding a new (sub)command the only thing that has to be done(aside from implementing the command) is adding {"<name>", spec, exec, check} to its parent command.

meta.c:

#define COMMAND_NAME "meta"

command metaSubcommands[] = {
	{ "ls", addMetaLsSpec, checkMetaLsArgs, execMetaLs },
};

...
void addMetaSpec (KeySet * spec)
{
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME), KEY_META, "description", "Get the value of an individual key.",
				   KEY_META, "command", COMMAND_NAME, KEY_END));

    	for (unsigned long i = 0; i < sizeof (subcommands) / sizeof (subcommands[0]); ++i)
	{
		subcommands[i].addSpec (spec);
	}
}
...
int execMeta(KeySet * options, Key * errorKey) {
	const char * subcommand = keyString (ksLookupByName (options, CLI_BASE_KEY "/" COMMAND_NAME, 0));

	for (unsigned long i = 0; i < sizeof (metaSubcommands) / sizeof (metaSubcommands[0]); ++i)
	{
		if (elektraStrCmp (subcommand, metaSubcommands[i].name) == 0)
		{
			return metaSubcommands[i].exec (options, errorKey);
		}
	}
}

meta-ls.c:

#define COMMAND_NAME "meta/ls"
...
void addMetaLsSpec (KeySet * spec)
{
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME), KEY_META, "description", "Get all meta information of an individual key.",
				   KEY_META, "command", "ls", KEY_END));
	
	ADD_BASIC_OPTIONS (spec, COMMAND_SPEC_KEY (COMMAND_NAME))
}
...

So, I'd use a KeySet instead of the command[], then instead of going through an array I would just keyLookup got get the fn pointers. But, if I understood this correctly, I need n lookups if I need the fn pointers of the nth subcommand( kdb <sub_1> ... <sub_n> ) since to get the name of the n+1th subcommand I need the data of proc:/CLI_BASE_KEY/<sub_1>/.../<sub_n>.

[kodebach]

I felt like it would make sense to be able to validate given data

Hm, yeah since you are using elektraGetOpts directly, there is probably no better way than a separate function. We could use two kdbGets, similar to what the current kdb does (at least sometimes). Then we could do what any other application would do and use gopts (via a contract passed to kdbOpen) and rely on Elektra's specification and plugin system, instead of manually verifying options.

I think @markus2330 might have had this "2 kdbGet & gopts" approach in mind all along for the new kdb.

Here the idea was that each subcommand takes care of all its subcommand

I guess that would work yes, but it's a bit annoying IMO since every command with further sub-commands would have to duplicate the logic for sub-commands.

I think the basic decision we have to make here is: Do we want

1. as much separation between the commands as possible?
2. a centralised specification for the entire kdb tool that only delegates where necessary?

[markus2330]

I think the basic decision we have to make here is: Do we want

as much separation between the commands as possible?
a centralised specification for the entire kdb tool that only delegates where necessary?

I think we need separation because separately implemented commands (in different processes) should look&feel as if they are internal commands (also be aware of common config like profiles, bookmarks etc.)

@hannes99 can you also describe or give a implementation sketch how external commands could have specifications and process arguments? E.g. with the already discussed kdb gen <args> (to be moved from src/tools/kdb/gen/ to src/tools/gen/)

Disclaimer: I didn't read the full discussion.

[kodebach]

If we use gopt via an initial kdbGet for the config of kdb, then external commands could just mount their spec as "sub-mountpoints" below the spec of kdb.

However, this would mean we need to properly mount the spec of kdb too. Which in turn means kdb needs to be installed correctly and can't just be run standalone.

I also agree that there should be some level of separation. But there should also a good level of cohesion, since we are trying to develop one tool after all. Maybe we can find a way to automatically read the entire spec from separate config files (one per command). I could live with splitting the spec into multiple INI (ni plugin) files. But having the spec defined in ksNew/keyNew calls is difficult enough to read, splitting it up across lots of files with no enforced system (the symbol for fooSpec could be exported from anywhere) is a bit much for my taste.

[hannes99]

@markus2330 that could look something like this:

gen.c

/**
 * @file
 *
 * @brief
 *
 * @copyright BSD License (see LICENSE.md or https://www.libelektra.org)
 */

#include <gen.h>

#include <command.h>
#include <kdbassert.h>
#include <kdbease.h>
#include <kdberrors.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>

#define COMMAND_NAME "gen"
#define EXTERNAL_PROGRAM "/usr/bin/echo"

#define GET_OPTION_KEY(options, name) GET_OPT_KEY (options, COMMAND_BASE_KEY (COMMAND_NAME) "/" name)
#define GET_OPTION(options, name) GET_OPT (options, COMMAND_BASE_KEY (COMMAND_NAME) "/" name)

void addGenSpec (KeySet * spec)
{
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME), KEY_META, "description", "Elektra's code-generator", KEY_META,
				   "command", COMMAND_NAME, KEY_END));
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME) "/templatename", KEY_META, "description", "The name of the key",
				   KEY_META, "args", "indexed", KEY_META, "args/index", "0", KEY_END));
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME) "/parentkey", KEY_META, "description", "The name of the key", KEY_META,
				   "args", "indexed", KEY_META, "args/index", "1", KEY_END));
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME) "/outputname", KEY_META, "description", "The name of the key", KEY_META,
				   "args", "indexed", KEY_META, "args/index", "2", KEY_END));
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME) "/parameters/#", KEY_META, "description",
				   "a list of parameters, the supported parameters depend on the template", KEY_META, "args", "remaining",
				   KEY_END));
	ksAppendKey (spec, keyNew (COMMAND_SPEC_KEY (COMMAND_NAME) "/inputfile", KEY_META, "description",
				   "Load a file with plugin instead of accessing the KDB", KEY_META, "opt", "F", KEY_META, "opt/arg/help",
				   "<plugin>=<file>", KEY_META, "opt/long", "inputfile", KEY_META, "opt/arg", "required", KEY_END));

	ADD_BASIC_OPTIONS (spec, COMMAND_SPEC_KEY (COMMAND_NAME))
}

int execGen (KeySet * options, Key * errorKey)
{
	Key * inFileKey = GET_OPTION_KEY (options, "inputfile");
	const char * template = GET_OPTION (options, "templatename");
	const char * parentkey = GET_OPTION (options, "parentkey");
	const char * output = GET_OPTION (options, "outputname");
	int argvIndex = 0;
	const char * genArgs[64] = {
		EXTERNAL_PROGRAM,
	};

	if (inFileKey != NULL)
	{
		genArgs[++argvIndex] = "-F";
		genArgs[++argvIndex] = keyString (inFileKey);
	}
	if (GET_OPTION_KEY(options, "verbose") != NULL)
	{
		genArgs[++argvIndex] = "-v";
	}
	if (GET_OPTION_KEY (options, "debug") != NULL)
	{
		genArgs[++argvIndex] = "-d";
	}

        ...

	genArgs[++argvIndex] = template;
	genArgs[++argvIndex] = parentkey;
	genArgs[++argvIndex] = output;

	Key * parametersParent = GET_OPTION_KEY (options, "parameters");
	KeySet * params = elektraArrayGet (parametersParent, options);
	Key * cur = NULL;
	for (elektraCursor it = 0; it < ksGetSize (params); ++it)
	{
		cur = ksAtCursor (params, it);
		genArgs[++argvIndex] = keyString (cur);
	}
	ksDel (params);
	genArgs[++argvIndex] = NULL;

	int status;

	EXEC_EXT (EXTERNAL_PROGRAM, genArgs, &status);

	if (status != WEXITSTATUS (status))
	{
		ELEKTRA_SET_INTERNAL_ERRORF (errorKey, "'%s' did not terminate properly", EXTERNAL_PROGRAM);
		return -1;
	}
}

int checkGenArgs (ELEKTRA_UNUSED KeySet * options, ELEKTRA_UNUSED Key * errorKey)
{
	// could check here if GET_OPTION(options, "inputfile") matches "<plugin>=<file>"
	return 0;
}

command.h, depending on where we'd need the EXEC_EXT macro we could also put it somewhere less general:

...
#define GET_OPT_KEY(options, key) ksLookupByName (options, "proc:" key, 0)
#define EXEC_EXT(prog, argv, status)                                                                                                       \
	pid_t extPid;                                                                                                                      \
	int timeout = 1000;                                                                                                                \
	if (0 == (extPid = fork ()))                                                                                                       \
	{                                                                                                                                  \
		if (-1 == execve (prog, (char **) (argv), NULL))                                                                             \
		{                                                                                                                          \
			perror ("child process execve failed [%m]");                                                                       \
			return -1;                                                                                                         \
		}                                                                                                                          \
	}                                                                                                                                  \
	while (0 == waitpid (extPid, status, WNOHANG))                                                                                    \
	{                                                                                                                                  \
		if (--timeout < 0)                                                                                                         \
		{                                                                                                                          \
			perror ("timeout");                                                                                                \
			return -1;                                                                                                         \
		}                                                                                                                          \
		sleep (1);                                                                                                                 \
	}
...

in main.c we'd just add

...
	{ "gen", addGenSpec, execGen, checkGenArgs },
...

We could probably also catch stdout of exec and if it does not exit properly we can wrap it using ELEKTRA_SET_INTERNAL_ERRORF, so main would be the only place that prints errors, but not sure if that makes sense.

kdb --help:

Usage: kdb [OPTION...] [COMMAND [...]]

OPTIONS
  --help                      Print this help message

COMMANDS
  gen                         Elektra's code-generator

kdb gen --help:

Usage: kdb gen [OPTION...] <outputname> <parentkey> <templatename> [<parameters>...]

OPTIONS
  --help                      Print this help message
  -c WHEN, --color=WHEN       Print never/auto(default)/always colored output.
  -d, --debug                 Give debug information or ask debug questions (in interactive mode).
  -F <plugin>=<file>, --inputfile=<plugin>=<file>
                                Load a file with plugin instead of accessing the KDB
  -n, --no-newline            Suppress the newline at the end of the output.
  -p NAME, --profile=NAME     Use a different profile for kdb configuration.
  -v, --verbose               Explain what is happening.
  -V, --version               Print version info.

PARAMETERS
  outputname                  The base name of the output files.
  parameters...               a list of parameters, the supported parameters depend on the template
  parentkey                   the  parent  key  to use, templates may have certain restrictions.
  templatename                Name of the template. More info `man kdb-gen`

Side note

The help message of kdb gen --help has the order of the positional args wrong, they are however parsed correctly(in the correct order) so it just seems to be some help message generation thing. Which I'll take a look at.

[hannes99]

I could live with splitting the spec into multiple INI (ni plugin) files.

Would then the compiled bin rely on specs in external config files? The implementations of commands depend on a specific specification for options and args, I feel like allowing the changing of those might not be a good thing. I think the spec should be baked into to final binary so commands can be sure the spec is as they expect. Or am I missing something? @kodebach

[kodebach]

The implementations of commands depend on a specific specification for options and args, I feel like allowing the changing of those might not be a good thing.

That's one of the things I tried to solve with the very much failed specload plugin.

also be aware of common config like profiles, bookmarks etc.

If I understand correctly, for this to work, we need to keep this separate kdbGet call

libelektra/src/tools/kdb/cmdline.cpp

Line 298 in 14d6500

kdb.get (conf, dirname);

To make that work properly, we should really use gopts instead of calling elektraGetOpts directly.

However, for that to work, the spec:/ keys for the command-line options must be available during kdbGet. The easiest way is to mount the spec as a standard mountpoint. A slightly better way is something like specload, where the keys are loaded by executing a program (a similar plugin could also execute a function from a shared library for this). However, this still requires a mountpoint that is correctly configured. Any attempt to do this should also happen on the new-backend branch.

But the best idea would probably be to extend the kdbOpen contract support in gopts to allow passing not only argc, argv and envp, but also the spec itself. I wouldn't modify elektraGOptsContract for this, but I would add a separate function that could be used in addition to elektraGOptsContract:

/**
 * Sets up a contract for use with kdbOpen() that configures
 * the gopts plugin to use a hard coded specification instead of the one loaded from the KDB.
 * This is useful for applications that are very dependent on having a correct spec, or which must run standalone without creation of mountpoints.
 *
 * @param contract    The KeySet into which the contract will be written.
 * @param spec    The spec:/ keys that gopts will pass to elektraGetOpts.
 *
 * @retval -1 if @p contract is NULL
 * @retval  0 on success
 */
int elektraGOptsSpecContract (KeySet * contract, KeySet * spec)

---

For defining the spec, I'd personally still prefer having a single file with the entire spec for kdb and maybe some kind of mechanism to load the spec for external commands.

But I'm also fine with separating the spec into sections for each command, if it is done in a more readable way than what is currently proposed. Maybe something similar to what we do with plugin contracts (i.e. #include in the middle of ksNew):

/***** start of main.c *****/
#define KDB_SPEC_INCLUDE
KeySet * spec = ksNew (1,
    keyNew ("spec:" CLI_BASE_KEY "/verbose", KEY_META, "description", "Explain what is happening.", KEY_META, "opt", "v", KEY_META, "opt/long", "verbose", KEY_META, "opt/arg", "none", KEY_END),
    // ... other options for `kdb` itself ...
#include "get.spec_incl.c"
#include "set.spec_incl.c"
// ...
    KS_END
);
#undef KDB_SPEC_INCLUDE

/***** start of get.spec_incl.c *****/
#ifndef KDB_SPEC_INCLUDE
static KeySet * spec()
{
    return ksNew (1,
#endif
        keyNew ("spec:" CLI_BASE_KEY "/get", KEY_META, "description", "Get the value of an individual key.", KEY_META, "command", COMMAND_NAME, KEY_END),
        keyNew ("spec:" CLI_BASE_KEY "/all", KEY_META, "description", "Consider all of the keys", KEY_META, "opt", "a", KEY_META, "opt/long", "all", KEY_META, "opt/arg", "none", KEY_END),
        keyNew ("spec:" CLI_BASE_KEY "/name", KEY_META, "description", "The name of the key", KEY_META, "args", "indexed", KEY_META, "args/index", "0", KEY_END),
#ifndef KDB_SPEC_INCLUDE
    KS_END
    );
}
#endif

So the idea is there one main file that directly refers to the other parts of the spec (which could in turn refer to further file e.g. for kdb meta get, kdb meta set). I think this is a lot more readable and it is immediately obvious what needs to be modified to add a new command. In the current proposal you'd have to read and understand most of main.c (in particular the loops in main()) and probably look at an existing command to understand all the macros from command.h.

Note: The KDB_SPEC_INCLUDE stuff isn't used with plugin contracts and could be omitted. I just added, so that get.spec_incl.c by itself is still a valid C file that should compile on it's own.

[hannes99]

I guess I don't see the benefit of splitting the spec and implementation of a command. Does it not make sense to have one command as one file containing its spec and implementation? We could use keySetMeta to make it more readable. How would using gopts help with making it work properly? Couldn't we just load all the profile options into proc:/cli/kdb/<sub>/... and then overwrite them with whatever is set in argv? Or do you mean using gopts only to load profile/bookmarks etc.?

[markus2330]

Sorry, already had this reviewed a week ago but GitHub did not post it...

Your questions are now very detailed to the implementation. I would suggest that you first have a deeper look at the user interface as described in doc/help. Read over all of it to get a gist of the requirements. What needs to be changed (due to bugs or due to the new "sub command" interfaces)? What changes to you propose to make the interface nicer? Please create a separate PR (against new-backend. Later this one should also be against new-backend.) where you modify the docs with changes you propose. (In new-backend we can also merge these changes.)

[markus2330]

Do you plan to gradually replace commands? If not, it is probably makes sense to only include .h and .c.

[markus2330]

Please keep the same base key /sw/elektra/kdb/#0/current (the legacy /sw/kdb/profile/ can be removed!). I don't see why it should be done via a compile definition? At most it can be in a header but it is not really something anyone would change.

[markus2330]

This must be externally accessible (by other tools and also by kdb get spec:/sw/elektra/kdb/#0/current etc):

1. Either you use the specload plugin (which probably needs some love to work robustly), then your approach more-or-less works as is. Or
2. We use an external config file to be mounted to spec:/sw/elektra/kdb/#0/current. Then the (whole) spec should be in a .ni file.

[kodebach]

@hannes99 I think this should answer the need for gopts. If you don't use kdbGet for reading the spec within kdb, can't really be accessible externally.

We use an external config file to be mounted to spec:/sw/elektra/kdb/#0/current

I really wouldn't recommend that. Depending on the spec being correctly set up can be a problem in any application. But if the core tool kdb breaks, because a mountpoint got screwed up it would be extremely bad.

the (whole) spec should be in a .ni file.

Defining the spec as a ni plugin format file would be better than ksNew/keyNew, but if we want to bake it into the executable we'll run into bootstrap problems. I think this should only be future improvement, i.e. translate the spec via ni and c plugins during build.

[markus2330]

Only cascading lookups should be needed.

[kodebach]

Only cascading lookups should be needed.

With the current setup it doesn't matter, since elektraGetOpts only produces proc:/ keys. But with gopts using anything other than a cascading lookup here, is a straight-up bug.

[markus2330]

This macro name is imho even for a private header file too short. Please prefix all macros with ELEKTRA_ to be consistent with our coding style. See doc/CODING.md and doc/DESIGN.md

[markus2330]

continue after this error?

[markus2330]

Shouldn't be there if nothing was printed?

[markus2330]

This is probably also useful somewhere else? Then put it to some shared code. Otherwise make it static.

[markus2330]

Can you somehow mark code that you took 1:1 (only cout -> printf or similar) (or mark changes)? This would make the review easier.

[markus2330]

Please always add a very short description of the file (e.g. contains main routine with error handling)

[kodebach]

How would using gopts help with making it work properly?

There is already one reason above (externally accessible spec). Other reasons are:

• IIRC elektraGetOpts does not consider meta:/default at all.
• elektraGetOpts definitely doesn't handle any other spec like e.g. meta:/type
• AFAIK kdb's profile feature can be used to set default arguments within the KDB. That would automatically be handled correctly with gopts, since it would load the proc:/ keys at the right moment for all spec validation to work correctly (at least on new-backend it will) and a cascading lookup would return the correct key for everything.

[markus2330]

@hannes99 is there any open question?

[markus2330]

Please create a separate PR (against new-backend. Later this one should also be against new-backend.) where you modify the docs with changes you propose. (In new-backend we can also merge these changes.)

@hannes99 Please don't forget to target new-backend with this PR.

[hannes99]

Hey, @kodebach. Do you have an idea why here(in parseAndAddMountpoint):

libelektra/src/libs/elektra/kdb.c

Lines 625 to 648 in 3e9f5ed

	// load mountpoint level config
	Key * lookupHelper = keyDup (root, KEY_CP_NAME);
	keyAddBaseName (lookupHelper, "config");
	KeySet * systemConfig = ksBelow (elektraKs, lookupHelper);
	Key * configRoot = keyNew ("system:/", KEY_END);
	ksRename (systemConfig, lookupHelper, configRoot);
	keyDel (configRoot);
	// get the plugin list and remove the common prefix
	keySetBaseName (lookupHelper, "plugins");
	KeySet * plugins = ksBelow (elektraKs, lookupHelper);
	// open all plugins (replaces key values with Plugin *)
	if (!openPlugins (plugins, lookupHelper, modules, global, systemConfig, errorKey))
	{
	keyDel (mountpoint);
	keyDel (lookupHelper);
	ksDel (plugins);
	ksDel (systemConfig);
	return false;
	}

plugins is empty when the mountpoint uses specload(which causes kdbOpen to fail)? It works on the master branch, but on new-backend it doesn't. Unfortunately I was not able to figure out what changes might cause this.

The mountpoints config seems fine(?)

λ ~/ kdb ls system:/elektra/mountpoints
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/config
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/config/path
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/errorplugins
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/errorplugins/#5#noresolver#noresolver#
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/getplugins
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/getplugins/#0#noresolver
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/getplugins/#5#specload#specload#
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/getplugins/#5#specload#specload#/config
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/getplugins/#5#specload#specload#/config/app
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/mountpoint
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/setplugins
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/setplugins/#0#noresolver
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/setplugins/#5#specload
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/setplugins/#6#sync#sync#

and it was mounted with
sudo kdb mount -R noresolver specload.eqd spec:/sw/elektra/kdb/current specload "app=/home/hannes/git/libelektra/cmake-build-debug/bin/kdb"

[kodebach]

The mountpoints config seems fine

No they're not. That's one of the big changes in new-backend. The structure of system:/elektra/mountpoints is totally different.

and it was mounted with [...]

I assume the kdb executable you used was built from master. On new-backend the kdb mount code should already be updated an produce the correct config.

For the new-backend branch the config should look something like this

system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/plugins/noresolver/name = noresolver
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/plugins/specload/name = specload
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/plugins/specload/config/app = /home/hannes/git/libelektra/cmake-build-debug/bin/kdb
system:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/plugins/backend/name = backend

sytem:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/definition/path = specload.eqd

sytem:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/definition/positions/get/resolver = noresolver
sytem:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/definition/positions/get/storage = specload

sytem:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/definition/positions/set/resolver = noresolver
sytem:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/definition/positions/set/storage = specload
sytem:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/definition/positions/set/commit = noresolver
sytem:/elektra/mountpoints/spec:\/sw\/elektra\/kdb\/current/definition/positions/set/rollback = noresolver

[hannes99]

Ohhh, thanks! Works now.

Not sure if already written somewhere, but having mounpoints created with the old kdb will make stuff crash when having them around after updating to the new backend.

[kodebach]

having mountpoints created with the old kdb will make stuff crash

I don't think it's written explicitly somewhere, but it's known. I guess we should have some sort of migration script that transforms the old config to new one. I'll see what I can come up with. Shouldn't be too hard to do.

[hannes99]

Something else, if the spec(mounted with specload) is in spec:/sw/elektra/#0/current/... then gopts will put the command-line options in proc:/sw/elektra/#0/current/... and cascading lookups should work out of the box if no profile is specified(config would be in /sw/elektra/#0/current/). If a profile is specified in the command-line options then the location where gopts puts them(.../current/...) and the profile with a config possibly setup(.../<profile_name>/...) have different keynames, so we would have to lookup a key in /sw/elektra/#0/current/ first and then, if it is not there, in /sw/elektra/#0/<profile_name>/. But, now with cascading lookups we can't really tell if the key we get is from the command-line(so in proc) or comes from the default profile, which we have to ignore since a profile is explicitly specified.
The only thing I came up with is limiting the lookup to proc if a profile is specified, and after not finding it looking in the specified profile. I feel like there has to be a better solution for this, maybe rename the <profile_name> in the profile key to current(just in mem) but AFAIK that only works with non-cascading keys, sooo... Or maybe put the spec somewhere else? Not sure, do you have an idea? Do I miss something? @kodebach

[kodebach]

But, now with cascading lookups we can't really tell if the key we get is from the command-line(so in proc) or comes from the default profile, which we have to ignore since a profile is explicitly specified.

You can tell which namespace a Key is in after doing a cascading lookup, e.g.:

Key * found = ksLookup (ks, "/foo", 0);
if (keyGetNamespace (found) == KEY_NS_PROC) {
  // key is in proc namespace
}

Another solution would be to just rename everything from proc:/sw/elektra/#0/current/ to proc:/sw/elektra/#0/<profile>:

Key * currentParent = keyNew ("proc:/sw/elektra/#0/current", KEY_END);
Key * profileParent = keyNew ("proc:/sw/elektra/#0", KEY_END);
keyAddBaseName (profileParent, profile);
ksRename (ks, currentParent, profileParent);

Then you can just do a cascading lookup with /sw/elektra/#0/<profile>/foo.

But both of this solutions have drawbacks, so I'd say: Make it work for /sw/elektra/#0/current/ and ignore profiles for now. Once you have code for that, pushed it, so we can review the overall concept, before we get into the profile stuff.

I'm not even sure we do really need the profiles concept at all (@markus2330 what do you think?). If we do want profiles, it might be worth thinking about whether they should be handled more generally, so that other application can use this mechanism too.

[hannes99]

short changelist:

• now based on new-backend
• uses specload to make spec available externally (and for mounting it)
• uses gopts
• resolve bookmarks

[lgtm-com]

This pull request fixes 1 alert when merging 55693e4 into 3e9f5ed - view on LGTM.com

fixed alerts:

• 1 for FIXME comment

[hannes99]

jenkins build libelektra please

[kodebach]

Looks good so far, just a few notes. One important thing: When dealing with key names, always prefer using the key* APIs, since they do a lot of validation to make sure the name is valid. Only do direct string manipulation when there is no other way of doing things or you definitely cannot do things wrong (e.g. your getCascadingName is fine)

[hannes99]

Thanks for the feedback @kodebach !

[lgtm-com]

This pull request fixes 1 alert when merging 54b4a85 into 3e9f5ed - view on LGTM.com

fixed alerts:

• 1 for FIXME comment

[markus2330]

I'm not even sure we do really need the profiles concept at all (@markus2330 what do you think?). If we do want profiles, it might be worth thinking about whether they should be handled more generally, so that other application can use this mechanism too.

Similar to gopts, profiles shouldn't be hardcoded in the kdb application. Instead we should use an improvement of the profile plugin https://www.libelektra.org/plugins/profile (what is missing that it somehow works together with gopts: I created #4481).