[new-backend] Open questions (part 2)

[kodebach]

This is a summary of #4407 with some extra questions.

Important: If you want to comment on one of the open questions, please always use the title of the question used in this description, even if there is a previous comment you're replying to. This makes it easier to track conversations. For example:

## `modules` vs `global`

> Some "previous response"

I disagree with "previous response", because ...

## `elektraModulesInit` error

I think ...

modules vs global

Question

Should we merge the KeySet * modules and KeySet * global within struct _KDB?

Summary

• The keysets already use separate hierarchies, so there should be no overlap.
• A benefit of merging would be reduced memory consumption. In struct _KDB we would have one less field and we'd need to allocate one less struct _KeySet. Beyond that all plugins that load extra plugins themselves (for whatever reason) would have access to this and wouldn't need to create a separate KeySet * modules.

  Note: Whether plugins should be able to load extra plugins is another question (see below), but this could be limited via the elektraPlugin* API.

Result

No result yet

Replace .dir with .elektra

Question

Should we use .elektra instead of .dir to store dir:/ keys.

Summary

• .dir is very generic and could cause conflicts. .elektra is more specific and reminds of .git

Result

No result yet

elektraModulesInit error

Question

Would it be better, if elektraModulesInit set an error directly, instead of letting the calling function define the error?

libelektra/src/libs/elektra/kdb.c

Lines 364 to 383 in 5f0895b

	static KDB * kdbNew (Key * errorKey)
	{
	KDB * handle = elektraCalloc (sizeof (struct _KDB));
	handle->modules = ksNew (0, KS_END);
	if (elektraModulesInit (handle->modules, errorKey) == -1)
	{
	// TODO (kodebach) [Q]: shouldn't we let elektraModulesInit set this error?
	ELEKTRA_SET_INSTALLATION_ERROR (
	errorKey, "Method 'elektraModulesInit' returned with -1. See other warning or error messages for concrete details");
	ksDel (handle->modules);
	elektraFree (handle);
	return NULL;
	}
	handle->global =
	ksNew (1, keyNew ("system:/elektra/kdb", KEY_BINARY, KEY_SIZE, sizeof (handle), KEY_VALUE, &handle, KEY_END), KS_END);
	handle->backends = ksNew (0, KS_END);
	return handle;
	}

Summary

• Everyone agrees the current solution is a bad approach and should change.

Result

Make elektraModulesInit set an error directly.

KeySet * global as argument to elektraPluginOpen

Question

Should elektraPluginOpen take the global keyset as an argument?

Summary

• Currently after every elektraPluginOpen call in libelektra-kdb we need to do a plugin->global = kdb->global, otherwise the plugin is not initialized correctly. @atmaxinger already in encountered this oddity in a recent PR.
• Plugins can't access the global keyset in the open function right now, because that is called in elektraPluginOpen before plugin->global is set.
• elektraPluginOpen already takes two different KeySet* arguments, another would be even more API smell.

Result

No result yet.

Plugins loading extra plugins

Question

Should plugins (any kind: backend, hook, storage, etc.) be allowed to load other plugins via e.g. elektraPluginOpen?

Summary

• Plugins randomly loading other plugins can have unexpected consequences to the user.
• Backend plugins already should use elektraPluginFromMountpoint to get the Plugin * that was loaded by libelektra-kdb based on the mountpoint declaration. This could be extended to other plugins, i.e. all required plugins must be declared in system:/elektra/mountpoints/<mp>/plugins even those transitively needed.
• It is unclear, whether there is a use-case where elektraPluginFromMountpoint could not be used. e.g. when the KeySet * config for the newly loaded plugin can only be known at runtime.

Result

No result yet.

elektraPluginFromMountpoint API

Question

Should elektraPluginFromMountpoint return a Plugin *?

Summary

• Backend plugins need to be able to call other plugins.
• The fact that libelektra-kdb preloads the Plugin * allows early error detection and takes some burden off of backend plugins.
• Returning a full Plugin * for another plugin is not ideal. It allows things like doing elektraPluginSetData (otherPlugin, NULL).
• A different solution would to return a new type e.g. PluginCallHandle *, that only allows calling functions exported by the other plugin and nothing else.

Result

No result yet.

elektraPluginFromMountpoint Internals

Question

How should elektraPluginFromMountpoint access the plugins?

Summary

• Currently, we use the global KeySet. libelektra-kdb sets system:/elektra/kdb/backend/plugins correctly, before it calls a backend plugin.
  

  libelektra/src/libs/elektra/kdb.c

  Lines 1391 to 1394 in afeacc1

  	// TODO (kodebach): find a better way to pass plugins, global shouldn't be specific to the mountpoint
  	ksAppendKey (backendData->backend->global,
  	keyNew ("system:/elektra/kdb/backend/plugins", KEY_BINARY, KEY_SIZE, sizeof (backendData->plugins), KEY_VALUE,
  	&backendData->plugins, KEY_END));

• Using global is not ideal, because it's contents should not be specific to the mountpoint.

Result

No result yet.

Modifying parentKey value after resolver phase

Question

Should the parentKey value be changeable after the resolver phase?

Summary

• Currently, the docs state (doc/dev/kdb-operations.md & doc/dev/backend-plugins.md) that the value of the parentKey can only be modified during the resolver phase. This makes sense, because this is the phase when it should be decided what "storage unit" (e.g. file) shall be used.
• The fcrypt plugin wants to change the value (during get), even though it executes only during prestorage and postorage for get and during precommit for set. The reason is: fcrypt switches from the actual file, which is encrypted, to a temporary decrypted file.
• libelektra-kdb currently does not lock the parentKey value at all during get. This is marked with a TODO [new_backend] and fcrypt workaround.

Result

No result yet.

[kodebach]

KeySet * global as argument to elektraPluginOpen

If plugins are not allowed to load extra plugins, I would add the KeySet * global argument and make elektraPluginOpen internal/private. It is still API smell but for non-public APIs with limited use that is not so bad.

Otherwise, I would change the API to:

/**
 * Opens a new plugin.
 *
 * The config will be used as is. So be sure to transfer ownership
 * of the config to it, with e.g. ksDup().
 * elektraPluginClose() will delete the config.
 *
 * @param handle handle of an existing loaded plugin, required to propagate internal and shared data
 * @param name name of the new plugin to open
 * @param config configuration for the new plugin to open
 * @param errorKey Key to store errors
 * 
 * @return a pointer to a new created plugin or 0 on error
 */
Plugin * elektraPluginOpen (Plugin * handle, const char * name, KeySet * config, Key * errorKey);

This could be used by plugins directly and libelektra-kdb has private-API access and can just stack allocate a fake struct _Plugin with the require data that will copied. Concretely, the Plugin * handle would be used for KeySet * modules and KeySet * global.

[kodebach]

elektraPluginFromMountpoint Internals

An easy solution is to add a KeySet * plugins to struct _Plugin instead of hiding it in the global KeySet.

Another solution would be to add a KDB * into struct _Plugin and use that somehow. This KDB * could also replace KeySet * global and KeySet * modules. Since a Plugin * is always linked to a KDB * instance making the link explicit seems only natural.

[kodebach]

Modifying parentKey value after resolver phase

IMO the value should be locked by libelektra-kdb after the resolver phase, because that just makes sense.

The easy option for fcrypt would of course be, if it actually decrypted the file indicated by the resolver. But I'm sure there is a reason it doesn't do this already.

So to handle fcrypt, the backend plugin should be extended:

1. Add a postresolver position with contract placements postgetresolver and postsetresolver
2. The postresolver position can have an unlimited number of plugins (like e.g. poststorage)
3. The postresolver plugins are executed after the plugin in the resolver position during the resolver phase
4. When calling a postresolver phase the meta:/backend/resolver/status will be set to the status returned by the resolver
5. The backend plugin always returns the value it got from the resolver. The value produces by postresolver plugin(s) is only stored internally. If it differs from the resolver one, backend creates it's own internal copy of parentKey to call plugins in later phases.

This would allow fcrypt to check, if the file will be read (via meta:/backend/resolver/status) and if so create the decrypted version and change the parentKey. The last point is needed to make kdb file work.

[github-actions]

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 💖

[github-actions]

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 💖