global-plugins: mark placements #874

mpranj · 2016-08-01T21:22:48Z

Introducing only a few of the proposed global plugin placements.

markus2330 · 2016-08-02T08:35:48Z

Can you mark the placements you found to be best nevertheless and write why they are not ideal? The foreach and init placements are also missing.

markus2330 · 2016-08-04T12:26:59Z

src/libs/elektra/kdb.c

@@ -447,6 +447,8 @@ int kdbClose (KDB * handle, Key * errorKey)
 */
 static int elektraGetCheckUpdateNeeded (Split * split, Key * parentKey)
 {
+	// GLOBAL: getresolver [init]


init could be even earlier directly in kdbGet?

markus2330 · 2016-08-04T12:38:12Z

There are some improvements! Maybe its better to directly work on the implementation now (instead of only having comments for placements) because you need to split some loops and some control flow needs changes because of deinit.

It would be good if you have a helper function/macro for executing the hooks so that the hooks are better visible/readable.

mpranj · 2016-08-04T16:38:09Z

@markus2330 thanks for the feedback. Yes indeed, I think it's better to switch to concrete code now.

Did you find anything that was completely misplaced in general? I was unsure about what semantics was wanted, but it will be unambiguous with concrete code anyway.

I was already planning on having a function or macro to keep the verbosity/boilerplate code about the same as the current comments (one-liners). It would introduce a lot of duplicate code otherwise.

markus2330 · 2016-08-05T20:32:39Z

src/libs/elektra/kdb.c

 	if (handle->globalPlugins[PRECOMMIT])
 	{
 		handle->globalPlugins[PRECOMMIT]->kdbSet (handle->globalPlugins[PRECOMMIT], ks, parentKey);
 	}

 	elektraSetCommit (split, parentKey);
+	// GLOBAL: commit [init]


should be before elektraSetCommit

or is even alias for precommit?

markus2330 · 2016-08-05T20:41:31Z

I already commented about some issues, but they are minor and it would also be trivial to fix the issues with code.

Maybe @tom-wa, @Namoshek, @waht or @KurtMi can find further problems, they already used (or even implemented) hooks.

Another approach would be to pinpoint problems within the test cases, maybe you can write some of them upfront? The desired sequence of called hooks in certain situations should be easy to check.

tom-wa · 2016-08-05T20:53:56Z

src/libs/elektra/kdb.c

+
+	// GLOBAL: postgetstorage [init]
+	// GLOBAL: postgetstorage [max once]
+


just for your information: elektraGetDoUpdatewithGlobalHooks gets called twice (thats why there's the run parameter)
you've used if statements for the set placements but not here, so i'm not sure if you've thought of that

Thanks. I did think of that since we discussed it in a meeting.

All of these are just rough markers where I thought the hooks could be placed.

mpranj · 2016-08-05T21:24:54Z

Then I don't think that it would make sense for anyone to review it yet, so don't waste your time.
It would be appreciated when the code is ready.

mpranj · 2016-11-17T02:05:16Z

This does not yet comply with the proposal for global plugins, but it adds some more positions which would be useful for #1072. (postgetstorage/deinit in particular)

@markus2330 would you consider merging the available part of the implementation already? Then I could continue working in another PR while @Namoshek could start using it already.

Namoshek · 2016-11-17T14:02:22Z

Not much success so far. I used postgetstorage/init as well as postgetstorage/deinit, but it doesn't seem to work. Could also be that I'm doing something wrong or my plugin does something wrong, but I don't know how to test it (with unit tests) as global plugin.

I had to change the last function in cmake/Modules/LibAddMacros.cmake by the way, it did not accept slashes for infos/placements. You could add this to your PR (replace [a-zA-Z0-9 ]*) by [a-zA-Z0-9/ ]*).

mpranj · 2016-11-17T16:42:53Z

@Namoshek, you have to either:

use kdb global-mount to mount it
or configure it manually like it's done in tests/shell/shell_recorder/globaltest.esr by setting the apropriate keys. Note that after setting the keys you need to kdbClose and then kdbOpen again. Otherwise the mount procedure is not done.

Yes, infos/placements is very misleading w.r.t. global plugins and (currently) has nothing to do with it.

Namoshek · 2016-11-17T17:14:27Z

Yes, infos/placements is very misleading w.r.t. global plugins and (currently) has nothing to do with it.

Ok, that info is (obviously) very important, thanks for pointing it out. Now I also understand why you want to inform the plugin in what stage it is executed (would definitely make sense to limit the cachefilter plugin here to one state, as it could potentially increase execution time enormously, although it shouldn't matter if it is executed 20 times (from a functional perspective)).

use kdb global-mount to mount it

That is what I did and I also confirmed it by listing the elektra keys with kdb ls system/elektra and getting the global plugin values (one is spec, the other cachefilter). So it should definitely be mounted. And of course I also restarted the service. I'll try once more and add some logging output to the plugin, maybe this helps.

Thank you so far!

mpranj · 2016-11-17T19:19:23Z

If it doesn't seem to work don't waste too much time. I'll look into it on saturday. Can't do any sooner. It's possible that the placement is not executed.

markus2330 · 2016-11-23T10:37:08Z

@mpranj Can you make this ready to merge? We now have two month to get rid of all issues.

mpranj · 2016-11-23T13:32:14Z

@markus2330 I'm pretty busy but it's already on my asap list. I'd expect it to be ready within a week.

Simplify and revamp mount.c to be able to mount the newly defined global positions. Use a macro to generate positions and subpositions. This makes the system more flexible to add/remove positions later. Use a function to execute global plugins and keep boilerplate code minimal. Add very simple and basic shell recorder test to test if mounting and executing the global plugins still works.

Calls the correct kdb operation on plugins (Get,Set,Error), was previously only kdbGet.

Add the elektraGlobal{Get,Set,Error} functions. This seemed more practical than using macros everywhere.

@Namoshek

thank to @Namoshek

markus2330 · 2017-01-02T22:17:37Z

src/plugins/cachefilter/cachefilter.c

@@ -42,7 +42,7 @@ int elektraCachefilterGet (Plugin * handle, KeySet * returned, Key * parentKey)
 	void * cache = elektraPluginGetData (handle);
 	if (cache == NULL)
 	{
-		toBeCached = ksNew (ksGetSize (returned), KS_END);
+		toBeCached = ksNew (0, KS_END);


Did you benchmark this? It might waste some space but should avoid many reallocations (for larger keysets).

No I didn't but this change is also not crucial.

The code of ksAppend, which is used later here, does suggest that it uses only one resize in advance.

I wanted to let the core decide about the allocation but if you're saying this is bad I don't mind undoing this. The objective was to get rid of the memleaks and move forward.

No I didn't but this change is also not crucial.

Thus I wondered why you changed it. Comments help in such situations.

But you are right, lets move forward for now. Before you start changing ksNew (to be suitable for mmap), it would be great to have benchmarks for such reallocations.

Yep you're right. I undid the change because it was unfounded.

mpranj · 2017-01-02T22:39:16Z

@markus2330 I'll close this PR today (I just kept it for the buildbots and some remaining infos).

Do you want me to do a PR on cachefilter or do you want to merge cachefilter to master if the memleaks are fixed now?

markus2330 · 2017-01-02T22:45:35Z

I'll close this PR today (I just kept it for the buildbots and some remaining infos).

Perfect, it is overdue!

Do you want me to do a PR on cachefilter or do you want to merge cachefilter to master if the memleaks are fixed now?

I am okay with both: You can create a single PR with all changes (and close #1078); or update #1078 and create a separate PRs for different modules. In any case, please keep changes of separate modules in separate commits.

mpranj added question work in progress labels Aug 1, 2016

markus2330 reviewed Aug 4, 2016
View reviewed changes

markus2330 reviewed Aug 5, 2016
View reviewed changes

tom-wa reviewed Aug 5, 2016
View reviewed changes

mpranj force-pushed the global-plugins branch from a6e4fa6 to 9eb7235 Compare September 22, 2016 09:04

mpranj force-pushed the global-plugins branch 2 times, most recently from f4a9715 to b7aa731 Compare October 4, 2016 20:01

mpranj force-pushed the global-plugins branch from b7aa731 to 180b432 Compare October 19, 2016 19:56

mpranj force-pushed the global-plugins branch 2 times, most recently from 48739f2 to c007bf0 Compare November 17, 2016 01:22

mpranj force-pushed the global-plugins branch from cfe12c1 to c3922a0 Compare November 17, 2016 09:23

Namoshek and others added 23 commits January 2, 2017 20:57

cachefilter plugin update

6e03342

global-plugins: mark placements

ae86b04

global plugins: make positions explicit (init, once, foreach, deinit)

af34548

global plugins: fixed call function

2d208ec

Calls the correct kdb operation on plugins (Get,Set,Error), was previously only kdbGet.

global plugins: simplify call

e964daf

global plugins: elektraGlobalGet

10b2564

Add the elektraGlobal{Get,Set,Error} functions. This seemed more practical than using macros everywhere.

global plugins: reformat code

dff293f

WIP: compiles

3e73055

WIP: compiles

3a878e2

WIP: compiles

c7810cd

WIP: compiles

90a9a99

WIP: compiles

9af5dc8

kdbglobal: fix warning

6cd5566

dbus: free+log on problems

6b03cc5

cmake: allow / in placements

e7507a8

thank to @Namoshek

dbus: reformat

03d12e1

cachefilter: reformat

70d0b5c

cachefilter: add logging

a601dcd

kdb: log problem

1d7d394

fix memproblems and invalid global spec config

5da15b6

global-plugins: this really fixes the memleak

a63e3d1

global-plugins: fix use after free

eaccfa5

mpranj force-pushed the global-plugins branch from 5557dd0 to eaccfa5 Compare January 2, 2017 20:12

markus2330 reviewed Jan 2, 2017

View reviewed changes

cachefilter: fix memleaks

b9cc18e

mpranj force-pushed the global-plugins branch from fc7e72b to b9cc18e Compare January 2, 2017 22:32

mpranj closed this Jan 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

global-plugins: mark placements #874

global-plugins: mark placements #874

mpranj commented Aug 1, 2016 •

edited

Loading

markus2330 commented Aug 2, 2016

markus2330 Aug 4, 2016

markus2330 commented Aug 4, 2016 •

edited

Loading

mpranj commented Aug 4, 2016

markus2330 Aug 5, 2016

markus2330 Aug 5, 2016

markus2330 commented Aug 5, 2016

tom-wa Aug 5, 2016 •

edited

Loading

mpranj Aug 5, 2016

mpranj commented Aug 5, 2016

mpranj commented Nov 17, 2016

Namoshek commented Nov 17, 2016

mpranj commented Nov 17, 2016

Namoshek commented Nov 17, 2016

mpranj commented Nov 17, 2016

markus2330 commented Nov 23, 2016

mpranj commented Nov 23, 2016

markus2330 Jan 2, 2017

mpranj Jan 2, 2017

markus2330 Jan 2, 2017

mpranj Jan 2, 2017

mpranj commented Jan 2, 2017

markus2330 commented Jan 2, 2017


		// GLOBAL: postgetstorage [init]
		// GLOBAL: postgetstorage [max once]

global-plugins: mark placements #874

global-plugins: mark placements #874

Conversation

mpranj commented Aug 1, 2016 • edited Loading

markus2330 commented Aug 2, 2016

Choose a reason for hiding this comment

markus2330 commented Aug 4, 2016 • edited Loading

mpranj commented Aug 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Aug 5, 2016

tom-wa Aug 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpranj commented Aug 5, 2016

mpranj commented Nov 17, 2016

Namoshek commented Nov 17, 2016

mpranj commented Nov 17, 2016

Namoshek commented Nov 17, 2016

mpranj commented Nov 17, 2016

markus2330 commented Nov 23, 2016

mpranj commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpranj commented Jan 2, 2017

markus2330 commented Jan 2, 2017

mpranj commented Aug 1, 2016 •

edited

Loading

markus2330 commented Aug 4, 2016 •

edited

Loading

tom-wa Aug 5, 2016 •

edited

Loading