Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Global plugins: global keyset handle #2307

Merged
merged 12 commits into from
Jan 23, 2019

Conversation

mpranj
Copy link
Member

@mpranj mpranj commented Jan 7, 2019

Basics

Adds a global keyset handle for global plugins #2270.
Get return value of global plugin calls.
See docs & release notes of PR.

Do not describe the purpose here but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains *(my name)*)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@mpranj mpranj changed the title WIP: global keyset handle Global plugins: global keyset handle Jan 8, 2019
@mpranj
Copy link
Member Author

mpranj commented Jan 8, 2019

@markus2330 please review my PR.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks very nice. Maybe the names are not ideal?

And please add some more documentation (within src/plugins/doc/doc.h)

@@ -323,6 +323,11 @@ struct _KDB

Plugin * notificationPlugin; /*!< reference to global plugin for notifications.*/
ElektraNotificationCallbackContext * notificationCallbackContext; /*!< reference to context for notification callbacks.*/

KeySet * global; /*!< This keyset can be used by global plugins to pass data through
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need it here if it is actually only a local variable within kdbGet/kdbSet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to have ksNew() only once per kdbOpen() and then ksClear() it before return such that plugins do not have to think about cleaning up.

As discussed in a meeting, part of it would be persisted in the cache, but this is for a different PR.

@@ -108,7 +108,8 @@ Plugin * elektraPluginExport (const char * pluginName, ...);
KeySet * elektraPluginGetConfig (Plugin * handle);
void elektraPluginSetData (Plugin * plugin, void * handle);
void * elektraPluginGetData (Plugin * plugin);

void elektraPluginSetGlobal (Plugin * plugin, KeySet * ks);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a correct name? If it is only available via a single invocation it is not exactly global, is it?

Maybe elektraPluginSetSharedData?

(And elektraPluginSetGlobalData for actually globally available data within a KDB handle?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this could be misleading terminology. Maybe I can come up with something better.

For my usecase, I only need what I described in doc/decisions/global_plugins.md.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe elektraPluginGetGlobalDataOfCurrentInvocation fits better? (and elektraPluginGetGlobalData if it is really global)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about what makes sense and reworked it to be global within a KDB handle and called it elektraPluginGetGlobalKeySet.

@mpranj
Copy link
Member Author

mpranj commented Jan 20, 2019

I am done with the changes.
@markus2330 please review my PR.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, very nice!

get a handle to a global keyset. The global keyset is tied to a KDB handle, initialized on kdbOpen() and
deleted on kdbClose().

The resolver plugin is an exception and also gets a handle to the global keyset. This way it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not every plugin?

@@ -127,7 +127,7 @@ compiled against an older 0.8 version of Elektra will continue to work

### Core

- <<TODO>>
- Global plugins now get a handle to a global keyset, which makes communication between global plugins easier. *(Mihael Pranjić)*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

", for communication between plugins"?

@@ -413,6 +416,11 @@ struct _Plugin

void * data; /*!< This handle can be used for a plugin to store
any data its want to. */

KeySet * global; /*!< This keyset can be used by global plugins and the resolver
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why special handling of resolver?

/**
* @brief Get a pointer to the global keyset.
*
* Only initialized for global plugins and the resolver.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

* Plugins using this keyset are responsible for cleaning up
* their parts of the keyset which they do not need any more.
*
* If kdbOpen() was not called earlier, NULL will be returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen?

@@ -102,6 +102,20 @@
* Note that you also need to return -1 in the case of error.
* See individual description of entry points to implement below.
*
* @par Global KeySet Handle
*
* Global plugins and the resolver will get a handle to a global keyset.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is it constructed and deleted?

@markus2330 markus2330 merged commit 8f15463 into ElektraInitiative:master Jan 23, 2019
@markus2330
Copy link
Contributor

Thank you! Please put the fixes (and further extensions) in a new PR.

@mpranj mpranj mentioned this pull request Jan 28, 2019
9 tasks
@mpranj mpranj mentioned this pull request Mar 30, 2019
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants