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