Re: Issue 130 in klish: Init hook and client cookie issues - point 1 implementation

[serj.k...@gmail.com]

I saw the diff to separate builtin but have no time yet to inspect and integrate it. I will
About contribution - I see two ways:
1. You have push access on klish project but don't change master, 1.5, 1.6 branches. These are "stable" or "main" branches. You use and create any your own branches on server and then write me about changes. I'll inspect and merge it to stable branches.
2. You can create a git repository clone (google allows to do it) and then i use your repository as remote server to integrate changes.

Anyway I must have full control over stable branches because the klish project is not a project for fun but a important part of our big project - the system that use klish as main control engine. So any changes must not break compatibility and other things. Line of development is defined by this big project needs. Of course any usefull changes that don't break anything will be applied.

So choose a workflow beetween these two possibilities.
You write "we". Are you not the only who will commit?



05.04.2013 12:24, Stanislav Galabov пишет:

Hi Serj,

1. Agreed, let's leave it as you committed it for now. If we need to use it somewhere else some day we'll think about the best way to proceed.

2. I had missed your 'if (res > 0) /* No chance to find name */ break;' the first time I read your code :-) Now that I looked at it again - no objections.

3. The 'one less search' argument was what tipped it for me too - I agree.

4. True as well - let's leave that for when/if we need to do something about it.

I agree - from now on I'll try to write as much as possible to klish-dev directly.

About contributions - how do you see this happening? Should we be able commit changes ourselves or should all changes pass through you? I would personally prefer that we get a commit bit, but (at least initially) we won't commit anything before obtaining your review comments and approval. But one again - it's your call.

BTW, I am not sure if you saw, but I posted a diff that moves the builtin plugin to a separate subdir and builds it as a separate .so and also implements the default_plugin XML tag and what is needed for all of it to work. I've attached it to issue 130.

Best wishes,

Stanislav

On Apr 5, 2013, at 11:10 AM, "serj.k...@gmail.com" <serj.k...@gmail.com> wrote:

Hi Stanislav

The comments are welcome.

1. I agree If it's a general library. If these functions will be used multiply times in the project or some third party software will use udata. Does anybody need it? I think no (but may be i'm wrong). All "classes" (like var, command etc.) in clish/ dir is klish specific and nobody except clish_shell_... don't use it. Nobody will depend on libclish.so to use a ptype "class" independently.

I don't think it's necessary to change list structure (i mean your clish_udata_t struct). The structure of your clish_udata_var_t can be used.

In any case the better place for general libraries is lub/. The place for clish-independent re-usable subsystems. The lub stands for Little Usefull Bits and firstly was developed by Graeme McKerrell (the author of original clish project). Moreover he made liblub independent library (not part of clish project) for lastest clish versions. It's good to use liblub in different projects but it's not good for klish because the klish is main target for my version of liblub and these two projects must be developed in sync.

So it's not very important question for me. It's easier for me to read the code without superfluous entities. But if you consider udata as a real general subsystem you can use your way and put it to the lub/ dir.

2. It's little faster while search and slower while adding entries. The search is more important for klish. I agree it's not very faster in udata case. Is it too bad? What disadvantages do you see?

3. The user must free data. So the typical task is remove entry from udata list and then to free data. Why two searches instead one search?

4. I have thought about it too. Similar to point 1 if udata is general then free_callback is good decision. If udata is clish_shell specific then it's unimportant question.


It's very good about contribution. Welcome.
Let's write to klish-dev mailing list if you have no objections. It can help to another people to understand some klish internals.

05.04.2013 10:21, Stanislav Galabov пишет:

Hi Serj,

Thanks for committing this. I ran git head with my initial tests and they passed. I am not sure when I'll have time for more testing, but so far so good.

I have a number of observations/comments on the refactoring if you don't mind:

1. I initially designed the code to be self-contained in order to hide the internals (e.g., the use of lub_list as the data structure, etc.) from clients of the code and thus achieve better encapsulation. This way, should we need to change the data structure for example we only need to change it in udata/udata.c and nowhere else. With the committed changes we'll have to change things in each client (granted, currently the only client is shell, but what if we need to extend udata to others too?) This was the reason behind the separate structure with a single lub_list_t - the only thing the clients see is this structure (not even its contents) - so a client can't easily be smarter than it needs to be and fiddle with the object internals, which would make the object easier to maintain.

2. I am not so sure what benefits the sorted list gives over the unsorted list in this particular case - it doesn't guarantee faster searches for example… If we intend to optimise for search speed I'd look at another data structure (e.g., binary tree, r-b tree, etc.) - which brings me back to point 1 above :-)

3. I am not so sure about *_del_data() returning a void * to the stored data (although I can see how it may be useful). I guess my idea was that the client that created the data (e.g., a plugin) would take care to free it by calling *_get_data(), followed by either *_set_data() with NULL data or *_del_data() and then taking the responsibility to free the data (whenever needed or in a fini hook). This could, of course, be optimised with a single call to *_del_data(), but apart from potentially one less line of code and one less search in the list it does not appear to provide much more benefits. In any case, I don't object to *_del_data() returning a pointer to the deleted data, I'm simply making an observation.

4. For example, what if another plugin needs to del the data, but does not know how to free it or whether to free it at all (it may be statically allocated for example)? It may be a better idea to do it as they do it in PAM - in the *_set_data() method we could have an optional free_callback argument which, if not NULL, will be called on every operation that needs to free the data (e.g., a set with different data or a del). This way the proper callback will be called whenever data needs to be freed.

Having said all that - it's your decision after all :-)

In the meantime, I have talked to my company's upper management and I have obtained their permission to officially contribute code to klish an help you develop what we believe is a great project even further if you have no objections to that of course.

Please let me know what you think.

Best wishes,

Stanislav

On Apr 4, 2013, at 11:13 PM, "serj.k...@gmail.com" <serj.k...@gmail.com> wrote:

Hi
Thanks. The patch was applied with some refactoring. See the git head. The udata has structure like a plugin subsystem. Without separate struct with lub_list_t only.
IMHO it seems to be clearer and similar to another subsystems. Unfortunately today i had no time to test it.

04.04.2013 19:09, Stanislav Galabov пишет:

Hi Serj,

Understood and agreed. Here's a revised set of patches.
Please let me know what you think.

Best wishes,
Stanislav

On Apr 4, 2013, at 5:49 PM, "serj.k...@gmail.com" <serj.k...@gmail.com> wrote:

Hi Stanislav

Thanks for your patches!

I think the clish_udata_status_t enum is unnecessary. The NULL/non-NULL or 0/-1 is enough for these purposses.
And it will be better the clish_udata_get_data(clish_udata_t * instance, const char *name, void **value) and similar functions to return void * instead of argument void**.

04.04.2013 17:39, Stanislav Galabov пишет:

Hi Serj,

Attached you can find the diff I mentioned regarding user data implementation.
Currently I've made it as a separate class of object (in the way pargv, command, etc. are implemented) so it can be used by any part of klish.

At the moment I've only implemented support for it in shell (via shell_udata.c).
In shell_new.c I allocate new user data storage (in clish_shell_init()) and free it in clish_shell_fini().

Then the user can manage the user data using:
void *clish_shell__get_udata(clish_shell_t *instance, const char *name);
void clish_shell__del_udata(clish_shell_t *instance, const char *name);
void clish_shell__set_udata(clish_shell_t *instance, const char *name, void *data);

I've tested briefly using the plugin attached as test.c (BTW, did I mention that plugins were a great idea? :-) ) and tests passed.

There are probably rough edges to clean, but I'm willing to do the work if it's ok with you.

Please let me know what you think.

Best wishes,
Stanislav