On (16/04/13 10:50), Lukas Slebodnik wrote: >On (16/04/13 00:12), David Bambušek wrote: >>2013/4/12 Jakub Hrozek <jhro...@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@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
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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
