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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel