2013/4/12 Jakub Hrozek jhrozek@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_infoFor 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