[SSSD] Tool for querying SSSD database

Lukas Slebodnik lslebodn at redhat.com
Tue Apr 16 08:50:32 UTC 2013


On (16/04/13 00:12), David Bambušek wrote:
>2013/4/12 Jakub Hrozek <jhrozek at redhat.com>
>
>> On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:
>> > 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 corrected it, so it should be fine now.
>>
>> >
>> > >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){
>> >
>>
>That is clever, thanks for advice.
>
>> > -----------------------------------------------------------------------
>> > -----------------------------------------------------------------------
>> > 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.
>>
>
>I completely redesigned the tools, made it more general and also used hash
>table.
>
>>
>> In addition to Lukas' comments about the code I also wanted to comment on
>> the functionality:
>>
>> Currently the databases are only readable by root. You should either
>> restrict the tool so that it errors out if not ran by root, or handle
>> permission errors when opening the db gracefully.
>>
>> The tool must be able to understand and parse fully qualified user
>> names. Use the sss_parse_name() function for that, you can copy its
>> usage from the sss_cache tool. If the FQ names support is good enough, I
>> would consider dropping the -D parameter althogether.
>>
>
>done
>
>>
>> I think the default set of attributes displayed for the user should be
>> the same that getpwnam() returns. See the output of "getent passwd
>> $user".
>>
>
>done
>
>
>>
>> Currently the tool only prints the raw attribute names from ldb. Do you
>> also plan on printing human readable names ("GID Number" instead of
>> "gidNumber") ?
>>
>
>I redesigned all the prints, so now it id nicer for humans :)
>
>>
>> Some entries might not have "gecos" at all. In that case, fall back to
>> the cn value.
>>
>> There seems to be a bug when displaying domain info:
>> $ sudo ./sss_query -d -N ipaldap
>> There is no domain "ipaldap"!           <--- yes there is
>> Domain name: ipaldap
>> Domain provider: ldap
>> Domain version: 0.15
>> Domain's subdomains:                    <--- diplayed twice
>> Domain's subdomains:                    <---
>>
>> The "Domain's subdomains" probably shouldn't be displayed at all if
>> there are no subdomains.
>>
>> I saw some weird behaviour when I had multiple domains configured. When
>> I tried to query user from the first domain in the list sss_query first
>> printed the user, but then said "Object not found".
>>
>> Similarly when I wanted to query an object from the second domain, I had
>> to use the -D option to specify the exact domain. I would expect the tool
>> to iterate over the domains.
>>
>
>There was a bug that caused all this behavior, now it should be OK.
>
>>
>> The messages that are user visible must be marked as translatable. See
>> the definition of the ERROR() macro to see how to do that.
>>
>> When the tool is ready, it's going to require a man page and it needs to
>> be mentioned in sssd.spec.in so it's packaged in the RPMs correctly.
>>
>
>I made the man page and also put mention is sssd.spec.in.
>
>>
>> Please ask your thesis mentor if you can keep the copyright assigned to
>> yourself in the thesis. I vaguely remember I had to submit the copyright
>> to FIT VUT back in the day :-)
>>
>
>In progress, mentor is busy and not responding at the moment :)
>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>
>Thanks for all your advice and tips. I uploaded new version with new
>design, hopefully fixed all the bugs and added all the missing
>functionality. I will welcome all comments to new design or reports of
>mistakes.
>
>Thanks
>David Bambušek

That code is much more better, after redesign code is approximately 35%
shorter and much more readable.

In function print_object, you iterate over array of strings (user_attrs),
    foreach attr in user_attrs:
        int_value = get_value_from_hash(hash, attr)
        switch(int_value)

        .......
        case A_<type>:
            oa->key = g_strdup(SYSDB_<type>);
                               ^^^^^^^^^^^^
                              this is the same like a "attr"
            ret_value = talloc_strdup(mem_ctx,ldb_msg_find_attr_as_string(msg, SYSDB_<type>, NULL));
                                                                               ^^^^^^^^^^^^
                                                                        this is also "attr"
            strcpy(final, "Name:\t ");
                           ^^^^^^^^^
                 human readable version of "attr"
            break;

        ......


The purpose of hash_table was to reduce very long switch/case. Hash table will
help you with mapping one type to another and code will be much more simpler.

You can transform current hash table <string, int> (SYSDB_<type>, A_<type>)
to hash table <string, string> (SYSDB_type, "human readable version of SYSDB_<type>"
This approach should help you to remove switch/case from print_object function.


Also function search_object is longer then 250 lines, so you could try to:
  -- split it to two functions search_object, search_object_all
  -- or you could try to reuse function parameter "bool all"

LS



More information about the sssd-devel mailing list