Issue 130 in klish: Init hook and client cookie issues

[kl...@googlecode.com]

Status: New
Owner: ----
Labels: Type-Defect Priority-Medium

New issue 130 by stanisla...@smartcom.bg: Init hook and client cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

Attachments:
init_hook_and_client_cookie.diff 3.5 KB

--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[kl...@googlecode.com]

Updates:
Status: Started
Labels: -Type-Defect Type-Enhancement

Comment #1 on issue 130 by serj.k...@gmail.com: Init hook and client

cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #2 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #3 on issue 130 by serj.k...@gmail.com: Init hook and client

cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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).

[kl...@googlecode.com]

Comment #4 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

Attachments:
clish_test_hook.tgz 1.1 KB

[kl...@googlecode.com]

Comment #5 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #6 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

Attachments:
clish_plugin.diff 888 bytes

[kl...@googlecode.com]

Comment #7 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #8 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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.

[kl...@googlecode.com]

Comment #9 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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...

[kl...@googlecode.com]

Comment #10 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

Attachments:
clish_plugin_v2.diff 1.7 KB

[kl...@googlecode.com]

Comment #11 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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.

[kl...@googlecode.com]

Comment #12 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

Attachments:
clish_plugin_v3.diff 4.0 KB

[kl...@googlecode.com]

Comment #13 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #14 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #15 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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.

[kl...@googlecode.com]

Comment #16 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #17 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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.

[kl...@googlecode.com]

Comment #18 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

Best wishes,
Stanislav

Attachments:
clish_default_plugin.diff 46.7 KB

[kl...@googlecode.com]

Comment #19 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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.

[kl...@googlecode.com]

Comment #20 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Comment #21 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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.

[kl...@googlecode.com]

Comment #22 on issue 130 by stanisla...@smartcom.bg: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

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.

[kl...@googlecode.com]

Comment #23 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

Agree.

[kl...@googlecode.com]

Comment #24 on issue 130 by sgala...@gmail.com: Init hook and client cookie
issues
http://code.google.com/p/klish/issues/detail?id=130

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

[kl...@googlecode.com]

Updates:
Status: Fixed

Comment #25 on issue 130 by serj.k...@gmail.com: Init hook and client
cookie issues
http://code.google.com/p/klish/issues/detail?id=130

Done. Will be released since klish-1.7.0