[new-backend] Open questions (part 2) #4521

kodebach · 2022-10-08T13:56:18Z

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.

The text was updated successfully, but these errors were encountered:

kodebach · 2022-10-08T14:27:16Z

`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 · 2022-10-08T14:46:26Z

`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 · 2022-10-11T13:33:02Z

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:

Add a postresolver position with contract placements postgetresolver and postsetresolver
The postresolver position can have an unlimited number of plugins (like e.g. poststorage)
The postresolver plugins are executed after the plugin in the resolver position during the resolver phase
When calling a postresolver phase the meta:/backend/resolver/status will be set to the status returned by the resolver
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 · 2023-10-12T01:44:10Z

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 · 2023-10-26T01:44:14Z

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 💖

kodebach mentioned this issue Oct 8, 2022

[new-backend] Various open questions #4407

Closed

This was referenced Oct 8, 2022

New backend logic #4187

Merged

Tracking changes to the configuration within KDB #4514

Closed

atmaxinger mentioned this issue Oct 10, 2022

document decision process #4528

Merged

22 tasks

github-actions bot added the stale label Oct 12, 2023

github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[new-backend] Open questions (part 2) #4521

[new-backend] Open questions (part 2) #4521

kodebach commented Oct 8, 2022 •

edited

kodebach commented Oct 8, 2022

kodebach commented Oct 8, 2022

kodebach commented Oct 11, 2022

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 26, 2023

[new-backend] Open questions (part 2) #4521

[new-backend] Open questions (part 2) #4521

Comments

kodebach commented Oct 8, 2022 • edited

modules vs global

Question

Summary

Result

Replace .dir with .elektra

Question

Summary

Result

elektraModulesInit error

Question

Summary

Result

KeySet * global as argument to elektraPluginOpen

Question

Summary

Result

Plugins loading extra plugins

Question

Summary

Result

elektraPluginFromMountpoint API

Question

Summary

Result

elektraPluginFromMountpoint Internals

Question

Summary

Result

Modifying parentKey value after resolver phase

Question

Summary

Result

kodebach commented Oct 8, 2022

KeySet * global as argument to elektraPluginOpen

kodebach commented Oct 8, 2022

elektraPluginFromMountpoint Internals

kodebach commented Oct 11, 2022

Modifying parentKey value after resolver phase

github-actions bot commented Oct 12, 2023

github-actions bot commented Oct 26, 2023

kodebach commented Oct 8, 2022 •

edited

`modules` vs `global`

Replace `.dir` with `.elektra`

`elektraModulesInit` error

`KeySet * global` as argument to `elektraPluginOpen`

`elektraPluginFromMountpoint` API

`elektraPluginFromMountpoint` Internals

Modifying `parentKey` value after `resolver` phase

`KeySet * global` as argument to `elektraPluginOpen`

`elektraPluginFromMountpoint` Internals

Modifying `parentKey` value after `resolver` phase