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