Init hook and client cookie issues

[GoogleCodeExporter]

Hi all,

I have noticed several shortcomings in git head (as of today):
1. It is not possible for a plugin to set a client_cookie and reuse it later.
2. Even if a plugin registers an init hook it never gets called.
3. If dlopen or dlsym within clish_plugin_load() fail the error message 
produced in clish_shell_load_plugins() is far from informative (currently it 
just reports "Error: Can't load plugin %s.\n")

This hampers the ability to actually develop and debug klish plugins.
I have prepared a small patch (attached), which addresses these and I'd be 
happy if you include it in klish.

What version of the product are you using? On what operating system?
git head on FreeBSD

Please provide any additional information below.
Patch attached.

Best wishes,
Stanislav

Original issue reported on code.google.com by stanisla...@smartcom.bg on 3 Apr 2013 at 11:31

Attachments:

• init_hook_and_client_cookie.diff

[GoogleCodeExporter]

Hi

1. Thanks for the cookie reminder. But now I'm thinking about alternative 
cookie implementation like a PAM one. Each module can have its own named "data" 
that it can register using plugin API. It will be better because it's not 
obvious how to share single cookie beetween plugins.

2. I know. The git head is not stable. I'm working on it now... The plugin 
support and API is not finished.

3. I'll see it

Original comment by serj.kalichev@gmail.com on 3 Apr 2013 at 12:10

• Changed state: Started
• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Thanks, Serj,

If you need any help and if you think I can help - please let me know.
It's not that I have much time, but I'll try my best.

BTW, another observation - if you write a plugin and "forget" to define 
CLISH_PLUGIN_INIT within it, then after successfully calling dlopen() in 
clish_plugin_load() the call to dlsym() returns the address of 
CLISH_PLUGIN_INIT defined in clish/builtin/builtin_init.c

I can look into this if you want.

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 3 Apr 2013 at 12:30

[GoogleCodeExporter]

Thanks

About CLISH_PLUGIN_INIT. Are you sure? Do you see it experimentally or 
theoretically? I use handle of dlopen()'ed plugin for dlsym() but not a NULL 
(to see all dependence tree).

Original comment by serj.kalichev@gmail.com on 3 Apr 2013 at 1:05

[GoogleCodeExporter]

Hi, Serj,

Yes, I am sure, I saw it experimentally on FreeBSD at least (I can try Linux as 
well, if needed).
The issue is that the plugin is linked to libclish.so itself, so what happens 
is that when dlsym doesn't find the symbol it's looking for in the shared 
object (let's call it 'x') whose handle you've passed it, it continues to 
search through the shared objects that 'x' is linked with and finds it in 
libclish.so :-)

I'm attaching a .tgz with a Makefile, a .c file and 2 XML files (actually 
they're the files from xml_examples/test with a simple modification to 
startup.xml to reference the plugin).

If you add a printf to CLISH_PLUGIN_INIT in clish/builtin/builtin_init.c such 
as "printf("%s called\n", __func__);" and do the tests below you'll be able to 
see what I am talking about...

When you build the plugin with "make rebuild KLISH_LIB=-lclish" -> you'll see 
"clish_plugin_init called" twice.
When you build the plugin with "make rebuild" -> you'll see "clish_plugin_init 
called" only once.

I'll think a little more about how this can be solved.

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 3 Apr 2013 at 1:23

Attachments:

• clish_test_hook.tgz

[GoogleCodeExporter]

Confirmed on Linux as well (Ubuntu, I am not sure about its version).

Original comment by stanisla...@smartcom.bg on 3 Apr 2013 at 1:30

[GoogleCodeExporter]

Hi Serj,

I think I have a solution for the CLISH_PLUGIN_INIT issue (patch attached):
If this->file is NULL when calling clish_plugin_load() (which basically means 
that we're referencing ourselves) - call dlsym() with NULL handle; otherwise 
call it with RTLD_NEXT.
We still keep the handle returned by dlopen(), so we can properly dlclose() 
if/when needed.

The attached diff also contains my earlier changes (for point 3 in the original 
issue) to clish/plugin/plugin.c.

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 3 Apr 2013 at 1:45

Attachments:

• clish_plugin.diff

[GoogleCodeExporter]

Unfortunately it's not a solvation because klish can dlopen() multiple plugins 
so you'll get an init function of first plugin.

Original comment by serj.kalichev@gmail.com on 3 Apr 2013 at 2:26

[GoogleCodeExporter]

The linux's man says:
"... libraries should export routines using the __attribute__((constructor))  
and
       __attribute__((destructor)) function attributes. Constructor  routines  are  executed  before  dlopen()  returns,  and destructor routines are executed before dlclose() returns.".

Has FreeBSD such routines?
I'm not sure it's a best solution because it can have a portability problems.

Original comment by serj.kalichev@gmail.com on 3 Apr 2013 at 2:39

[GoogleCodeExporter]

FreeBSD man (at least in 8.1) doesn't mention this and, as far as I know, 
FreeBSD still relies on the _init()/_fini() method.

I agree with you - this is not the best solution in terms of portability...

Original comment by stanisla...@smartcom.bg on 3 Apr 2013 at 2:58

[GoogleCodeExporter]

How about this (patch attached):
If clish_plugin_load() gets called and this->file != NULL and the plugin_init 
returned by dlsym is == builtin CLISH_PLUGIN_INIT_FNAME -> error.
The only drawback is that I had to add CLISH_PLUGIN_INIT; to 
clish/plugin/private.h , but since this is plugin's private header that 
shouldn't be much of an issue... what do you think?

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 3 Apr 2013 at 3:47

Attachments:

• clish_plugin_v2.diff

[GoogleCodeExporter]

Yes, it was a first idea. But... it will be main reserved idea. It has a 
disadvantage.

All builtin functions and default hooks were moved to the builtin/ dir. All 
this code is not engine but something like an internal plugin. So it can be 
disabled while configuration (it will be). It's usefull for some exotic 
platforms and special cases that don't need shell code execution or config 
daemon and has its own hook implementation. So when the builtin hooks are 
disabled there is no internal INIT at all.

Another idea is something like CLISH_PLUGIN_INIT(myplugin) where myplugin is 
plugin name. But it's harder for XML PLUGIN including.

Original comment by serj.kalichev@gmail.com on 3 Apr 2013 at 5:52

[GoogleCodeExporter]

Hi, Serj,

I've attached an updated diff, which implements a --disable-internal-plugin 
option in configure.ac.
If the user doesn't select this option - then in config.h we have 
HAVE_INTERNAL_PLUGIN defined.
Also, in Makefile.am and module.am we export a variable INTERNAL_PLUGIN and if 
it tests true we compile the contents of the clish/builtin directory.
Then, in clish/plugin/plugin.c we have a stub CLISH_PLUGIN_INIT { return 0; } 
which is compiled only if HAVE_INTERNAL_PLUGIN is not defined.

I would prefer this approach personally, as anything else seems to add 
complexity, while not providing much other benefits, at least as far as I can 
see... what do you think?

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 3 Apr 2013 at 7:29

Attachments:

• clish_plugin_v3.diff

[GoogleCodeExporter]

I forgot to #include <config.h> in clish/plugin/plugin.c in the previous diff...

Original comment by stanisla...@smartcom.bg on 4 Apr 2013 at 6:27

[GoogleCodeExporter]

Hi, Serj,

BTW, regarding point 1 of the original issue, I have an implementation based on 
lub_list_t for this.
If you're interested - I can send it to you for review. If you want - after 
your review I can commit it too.

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 4 Apr 2013 at 12:13

[GoogleCodeExporter]

Hi. Good, send it (point 1).

I'm still thinking about right plugin implementation. The last idea is to 
convert internal plugin to real default plugin. So the dlsym() will return the 
real init function or NULL. Because libclish.so doesn't contain plugin's init 
and doesn't depend on any other plugin.

Original comment by serj.kalichev@gmail.com on 4 Apr 2013 at 1:23

[GoogleCodeExporter]

Regarding point 1 - I'll send the diffs to your email directly, if you don't 
mind. This way we can discuss it offline and, once polished, we can proceed 
with commit...

I agree with your idea on the internal plugin implementation - if it is built 
as a separate .so we won't see the problems we're seeing here and it will be 
much easier to 'override' or not use the internal plugin by simply saying (for 
example) use_builtin="plugin_name" in STARTUP tag or some similar mechanism...

Best wishes,
Stan

Original comment by stanisla...@smartcom.bg on 4 Apr 2013 at 1:30

[GoogleCodeExporter]

Yes, good, STARTUP tag can control the using (or not using) of default plugin. 
Probably the something like default_plugin="false" (or true) is better because 
the plugin_name is equal to common <PLUGIN file="..."/>. And STARTUP without 
any additional fields must use default plugin for compatibility so the default 
plugin name will be hardcoded anyway.

Original comment by serj.kalichev@gmail.com on 4 Apr 2013 at 1:48

[GoogleCodeExporter]

You mean something like the attached diff, right? :-)

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 4 Apr 2013 at 2:38

Attachments:

• clish_default_plugin.diff

[GoogleCodeExporter]

Hi

What do you think about clish_context_t * in plugin hooks and sym prototypes? 
For some hooks it's anough to pass clish_shell_t *, but builtin action syms 
(and may be some hooks) need all clish_context_t content. Additionally 
clish_context_t is transparent now. It's very good for internal code but not 
good for third party plugins.

By the way I have add clish_shell_t to CLISH_PLUGIN_INIT prototype to use 
something like udata within plugin init. And dlopen() uses RTLD_LOCAL now to 
don't pollute sym tree.

Original comment by serj.kalichev@gmail.com on 5 Apr 2013 at 11:41

[GoogleCodeExporter]

Hi Serj,

Regarding clish_context_t *, I think it is absolutely a good idea for plugin 
hooks. I am not certain about sym prototypes (haven't thought about it enough). 
For example, if you need to replace a config hook - it's a good idea to have 
all info required in one place and clish_context_t * provides just that. Not 
certain for the init/fini hooks, etc. but I guess it's a good idea that all 
hooks have the same function signature for simplicity. In any case it's easy 
enough to create a context with just a valid shell pointer in it in the places 
where no command/pargv is available or where it doesn't make sense to pass 
command/pargv to a hook.
And, possibly, we can extend the same to sym prototypes, just to be consistent.

As for transparency of shell_context_t - we should probably hide 
clish_context_s in shell/private.h and only export clish_context_t to external 
users. Of course - this would mean creating getter methods for pargv, shell, 
cmd and action, but it's better design IMHO.

As for adding the clish_shell_t to CLISH_PLUGIN_INIT - I was going to ask you 
about your opinion for the same thing :-) I think it's a good idea.

The same for dlopen with RTLD_LOCAL.

Best wishes,
Stanislav

Original comment by stanisla...@smartcom.bg on 5 Apr 2013 at 11:56

[GoogleCodeExporter]

Agree. I think something like that but was not sure.

Note CLISH_PLUGIN_INIT, CLISH_PLUGIN_FINI use clish_shell_t but not 
clish_context_t. I think it's not big problem because it's not a "clish hooks" 
but service functions only. Do you agree?

It's important to freeze good plugin API to don't change it in future and break 
compatibility.

Original comment by serj.kalichev@gmail.com on 5 Apr 2013 at 12:59

[GoogleCodeExporter]

In the case of CLISH_PLUGIN_INIT - seeing as it's called very early on (when 
there's absolutely no meaningful context) - clash_shell_t is more than enough 
in my opinion. Even if we pass full clish_context_t then everything else, 
besides the clish_shell_t, inside it would be meaningless.

I guess the same applies for FINI - it's called so late that there would most 
likely be no meaningful context.

I think PLUGIN_INIT/PLUGIN_FINI can be different than hook/sym callbacks as 
their roles are absolutely different.

Original comment by stanisla...@smartcom.bg on 5 Apr 2013 at 1:18

[GoogleCodeExporter]

Agree.

Original comment by serj.kalichev@gmail.com on 5 Apr 2013 at 1:31

[GoogleCodeExporter]

Hi Serj,

We could probably close this now as:
1. Udata solves the concerns of point 1 of the original issue in a more 
appropriate way than client_cookie did.
2. PLUGIN_INIT and PLUGIN_FINI are a much more appropriate mechanism to address 
point 2 of the original issue.
3. All the work on separating the built-in plugin addresses point 3 of the 
original issue.

Stanislav

Original comment by sgala...@gmail.com on 9 Apr 2013 at 1:45

[GoogleCodeExporter]

Done. Will be released since klish-1.7.0

Original comment by serj.kalichev@gmail.com on 9 Apr 2013 at 2:23

• Changed state: Fixed