[SSSD] Tool for querying SSSD database

Lukas Slebodnik lslebodn at redhat.com
Fri Apr 12 16:42:03 UTC 2013


On (12/04/13 01:31), David Bambušek wrote:
>Since I was really working with two months old version of SSSD, I made
>completely new mirror of the newest SSSD on my github account and made some
>changes in code of my sss_query, so it is compatible with it now. There
>should be no problem with compiling anymore.
>
>I also went through the code and changed it according to coding guidelines.
>About indentation, I use 4 spaces as coding style suggests, could you
>please specify where do I have it wrong? I also wrapped most of lines so
>they are not exceeding the 80 character limit, I just have kept some of
>them longer, cause in my opinion they would become unreadable, if wrapped,
>but still they are never longer than about 100 characters.

You spend much more time with reading code like writing code. So code have to
be readable.
80 columns is not about terminal limitations, but about code readability.
In most cases if you have line longer than 80 characters, you may doing
something wrong. Maybe you try to do a lot of thing on one line, or there are
a lot of nested blocks (if, while ...)

Some formatting issues, according to SSSD coding style:
http://www.freeipa.org/page/Coding_Style
    --white spaces at end of line
    --mixing tabs and spaces
I think that proper editor configuration could help you with both.

>
>I also corrected the memory leaks and memory handling through out the
>application, so hopefully it is alright now.
>
>David Bambušek
>

And some comments to code itself.

For each type you define as
#define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL }
#define <type>_SHOW_ATTRS_NUM number
...
    static const char *attrs[] = <type>_SHOW_ATTRS;

--You have to manually define array size, for each ATTR, it is error prone.
--You don't need to know array size, because each array is NULL terminated,

So you can transform it to something like this:

// much more readable and shorter then 80 columns
const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC,
                                     SYSDB_SSH_KNOWN_HOSTS_EXPIRE,
                                     SYSDB_SSH_PUBKEY,
                                     NULL };
...
const char **attrs = SSHHOST_SHOW_ATTRS;

And instead of while iteration with counter, you can benefit
from NULL terminated array. With this approach you can remove
parameters "int <type>_attrs_num" from all functions.
-    int counter = 0;
-    while (counter < user_attrs_num){
-        if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){
+    for (attr_iter = user_attrs; attr_iter; ++attr_iter){
+        if (strcmp(*attr_iter, SYSDB_NAME) == 0){

-----------------------------------------------------------------------
-----------------------------------------------------------------------
But most important think is that there is a lot of code duplication. For
example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS
I have to:
    --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS
    --add/remove member from struct sshhost_info
    --add/remove lines to search_user
    --add/remove lines to search_all_user (the same like in search user)
    --add/remove lines to print_user
It is also error prone.

You declare 6 structures: user_info, group_info, netgroup_info, service_info
                          autofsm_info, sshhost_info
For each structure you create functions with the same pattern.
    print_<type>_info
    search_<type>
    search_all_<type>

Both search function have a lot of similar code.
In search function you map <key, value> to structure members.
In print function you map structure members to <key, value> and
print formated <key, value>. This mapping is useless, because key and value
are strings. It would be easier to use hash_table instead of structures.
In that case, you can write general print, search, search_all function for all
types. SSSD already use hash_table from ding-libs package. Header file dhash.h

And at the end one positive:
Output from this utility is quite good.

LS



More information about the sssd-devel mailing list