[SSSD] Tool for querying SSSD database

Lukas Slebodnik lslebodn at redhat.com
Wed Apr 17 07:16:37 UTC 2013


On (16/04/13 23:46), David Bambušek wrote:
>2013/4/16 Lukas Slebodnik <lslebodn at redhat.com>
>
>> On (16/04/13 10:50), Lukas Slebodnik wrote:
>> >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.
>> >
>>
>OK, now I have finally understood how to use it correctly, thanks a lot, it
>is really a big line saver and much better solution than was mine.
>
>> >
>> >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
>>
>Functions are now split into three, one for certain object, one for all
>objects and one for domains and subdomains.
>
>>
>> I forgot to mention one important thing in previous email.
>>
>> You have used hash table from glib, but glib is optional dependency in
>> sssd.
>> For example in debian distribution, sssd does not depend on glib(libglib).
>> You should use hash table from ding-libs(libdhash)
>>
>> Fedora distribution have package libdhash-devel and you can find exmaples
>> in directory /usr/share/doc/libdhash-devel-0.4.3/
>>
>> LS
>>
>
>Sorry for that, I used it by mistake, already corrected.
>
>>
>>
>>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>
>So the newest version is already on my github, with all the issues
>mentioned above fixed.
>
>David Bambušek

#include <stddef.h>
#include <glib.h>
           ^^^
You forgot to remove header file
#include "dhash.h"

...

    if ( (entry_type == TYPE_DOMAIN) || (entry_type == TYPE_DOMAIN_ALL) || (entry_type == TYPE_SUBDOMAIN)) {
        print_domain_info(tctx, search, entry_type);   
    } else if (!all) {
        search_object(tctx, hashed_attrs, tctx, object_type, entry_type, protocol,
                      search, attribs, all, domain);
                                      ^^^^^
                                  every time false
You split search_object to two functions, so this argument is useless in this
function and should be removed and also related code from function
    } else {
        search_object_all(tctx, hashed_attrs, tctx, object_type, entry_type, attribs, domain);
    }


After splitting search_object -> {search_object, search_object_all}
enum sss_entry_type can be reduced by removing TYPE_<type>_ALL and
ALL_SHIFT should be also removed.

We don't like compile time warnings:
  CC       src/tools/sss_query-sss_query.o
src/tools/sss_query.c: In function ‘setup_hash’:
src/tools/sss_query.c:144:21: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default]
src/tools/sss_query.c: In function ‘search_object’:
src/tools/sss_query.c:389:29: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
src/tools/sss_query.c: In function ‘search_object_all’:
src/tools/sss_query.c:524:25: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
src/tools/sss_query.c:412:28: warning: unused parameter ‘type’ [-Wunused-parameter]
src/tools/sss_query.c: In function ‘delete_callback’:
src/tools/sss_query.c:541:61: warning: unused parameter ‘type’ [-Wunused-parameter]
src/tools/sss_query.c:541:73: warning: unused parameter ‘pvt’ [-Wunused-parameter]
src/tools/sss_query.c: In function ‘parse_attributes’:
src/tools/sss_query.c:556:22: warning: assignment from incompatible pointer type [enabled by default]


LS



More information about the sssd-devel mailing list