On (16/04/13 23:46), David Bambušek wrote: >2013/4/16 Lukas Slebodnik <lsleb...@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 <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. >> > >> >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@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel