<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2013/4/16 Lukas Slebodnik <span dir="ltr"><<a href="mailto:lslebodn@redhat.com" target="_blank">lslebodn@redhat.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On (16/04/13 10:50), Lukas Slebodnik wrote:<br>
>On (16/04/13 00:12), David Bambušek wrote:<br>
>>2013/4/12 Jakub Hrozek <<a href="mailto:jhrozek@redhat.com">jhrozek@redhat.com</a>><br>
>><br>
>>> On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:<br>
>>> > On (12/04/13 01:31), David Bambušek wrote:<br>
>>> > >Since I was really working with two months old version of SSSD, I made<br>
>>> > >completely new mirror of the newest SSSD on my github account and made<br>
>>> some<br>
>>> > >changes in code of my sss_query, so it is compatible with it now. There<br>
>>> > >should be no problem with compiling anymore.<br>
>>> > ><br>
>>> > >I also went through the code and changed it according to coding<br>
>>> guidelines.<br>
>>> > >About indentation, I use 4 spaces as coding style suggests, could you<br>
>>> > >please specify where do I have it wrong? I also wrapped most of lines so<br>
>>> > >they are not exceeding the 80 character limit, I just have kept some of<br>
>>> > >them longer, cause in my opinion they would become unreadable, if<br>
>>> wrapped,<br>
>>> > >but still they are never longer than about 100 characters.<br>
>>> ><br>
>>> > You spend much more time with reading code like writing code. So code<br>
>>> have to<br>
>>> > be readable.<br>
>>> > 80 columns is not about terminal limitations, but about code readability.<br>
>>> > In most cases if you have line longer than 80 characters, you may doing<br>
>>> > something wrong. Maybe you try to do a lot of thing on one line, or<br>
>>> there are<br>
>>> > a lot of nested blocks (if, while ...)<br>
>>> ><br>
>>> > Some formatting issues, according to SSSD coding style:<br>
>>> > <a href="http://www.freeipa.org/page/Coding_Style" target="_blank">http://www.freeipa.org/page/Coding_Style</a><br>
>>> > --white spaces at end of line<br>
>>> > --mixing tabs and spaces<br>
>>> > I think that proper editor configuration could help you with both.<br>
>>> ><br>
>>><br>
>>I corrected it, so it should be fine now.<br>
>>><br>
>>> ><br>
>>> > >I also corrected the memory leaks and memory handling through out the<br>
>>> > >application, so hopefully it is alright now.<br>
>>> > ><br>
>>> > >David Bambušek<br>
>>> > ><br>
>>> ><br>
>>> > And some comments to code itself.<br>
>>> ><br>
>>> > For each type you define as<br>
>>> > #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL }<br>
>>> > #define <type>_SHOW_ATTRS_NUM number<br>
>>> > ...<br>
>>> > static const char *attrs[] = <type>_SHOW_ATTRS;<br>
>>> ><br>
>>> > --You have to manually define array size, for each ATTR, it is error<br>
>>> prone.<br>
>>> > --You don't need to know array size, because each array is NULL<br>
>>> terminated,<br>
>>> ><br>
>>> > So you can transform it to something like this:<br>
>>> ><br>
>>> > // much more readable and shorter then 80 columns<br>
>>> > const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC,<br>
>>> > SYSDB_SSH_KNOWN_HOSTS_EXPIRE,<br>
>>> > SYSDB_SSH_PUBKEY,<br>
>>> > NULL };<br>
>>> > ...<br>
>>> > const char **attrs = SSHHOST_SHOW_ATTRS;<br>
>>> ><br>
>>> > And instead of while iteration with counter, you can benefit<br>
>>> > from NULL terminated array. With this approach you can remove<br>
>>> > parameters "int <type>_attrs_num" from all functions.<br>
>>> > - int counter = 0;<br>
>>> > - while (counter < user_attrs_num){<br>
>>> > - if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){<br>
>>> > + for (attr_iter = user_attrs; attr_iter; ++attr_iter){<br>
>>> > + if (strcmp(*attr_iter, SYSDB_NAME) == 0){<br>
>>> ><br>
>>><br>
>>That is clever, thanks for advice.<br>
>><br>
>>> > -----------------------------------------------------------------------<br>
>>> > -----------------------------------------------------------------------<br>
>>> > But most important think is that there is a lot of code duplication. For<br>
>>> > example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS<br>
>>> > I have to:<br>
>>> > --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS<br>
>>> > --add/remove member from struct sshhost_info<br>
>>> > --add/remove lines to search_user<br>
>>> > --add/remove lines to search_all_user (the same like in search user)<br>
>>> > --add/remove lines to print_user<br>
>>> > It is also error prone.<br>
>>> ><br>
>>> > You declare 6 structures: user_info, group_info, netgroup_info,<br>
>>> service_info<br>
>>> > autofsm_info, sshhost_info<br>
>>> > For each structure you create functions with the same pattern.<br>
>>> > print_<type>_info<br>
>>> > search_<type><br>
>>> > search_all_<type><br>
>>> ><br>
>>> > Both search function have a lot of similar code.<br>
>>> > In search function you map <key, value> to structure members.<br>
>>> > In print function you map structure members to <key, value> and<br>
>>> > print formated <key, value>. This mapping is useless, because key and<br>
>>> value<br>
>>> > are strings. It would be easier to use hash_table instead of structures.<br>
>>> > In that case, you can write general print, search, search_all function<br>
>>> for all<br>
>>> > types. SSSD already use hash_table from ding-libs package. Header file<br>
>>> dhash.h<br>
>>> ><br>
>>> > And at the end one positive:<br>
>>> > Output from this utility is quite good.<br>
>>><br>
>><br>
>>I completely redesigned the tools, made it more general and also used hash<br>
>>table.<br>
>><br>
>>><br>
>>> In addition to Lukas' comments about the code I also wanted to comment on<br>
>>> the functionality:<br>
>>><br>
>>> Currently the databases are only readable by root. You should either<br>
>>> restrict the tool so that it errors out if not ran by root, or handle<br>
>>> permission errors when opening the db gracefully.<br>
>>><br>
>>> The tool must be able to understand and parse fully qualified user<br>
>>> names. Use the sss_parse_name() function for that, you can copy its<br>
>>> usage from the sss_cache tool. If the FQ names support is good enough, I<br>
>>> would consider dropping the -D parameter althogether.<br>
>>><br>
>><br>
>>done<br>
>><br>
>>><br>
>>> I think the default set of attributes displayed for the user should be<br>
>>> the same that getpwnam() returns. See the output of "getent passwd<br>
>>> $user".<br>
>>><br>
>><br>
>>done<br>
>><br>
>><br>
>>><br>
>>> Currently the tool only prints the raw attribute names from ldb. Do you<br>
>>> also plan on printing human readable names ("GID Number" instead of<br>
>>> "gidNumber") ?<br>
>>><br>
>><br>
>>I redesigned all the prints, so now it id nicer for humans :)<br>
>><br>
>>><br>
>>> Some entries might not have "gecos" at all. In that case, fall back to<br>
>>> the cn value.<br>
>>><br>
>>> There seems to be a bug when displaying domain info:<br>
>>> $ sudo ./sss_query -d -N ipaldap<br>
>>> There is no domain "ipaldap"! <--- yes there is<br>
>>> Domain name: ipaldap<br>
>>> Domain provider: ldap<br>
>>> Domain version: 0.15<br>
>>> Domain's subdomains: <--- diplayed twice<br>
>>> Domain's subdomains: <---<br>
>>><br>
>>> The "Domain's subdomains" probably shouldn't be displayed at all if<br>
>>> there are no subdomains.<br>
>>><br>
>>> I saw some weird behaviour when I had multiple domains configured. When<br>
>>> I tried to query user from the first domain in the list sss_query first<br>
>>> printed the user, but then said "Object not found".<br>
>>><br>
>>> Similarly when I wanted to query an object from the second domain, I had<br>
>>> to use the -D option to specify the exact domain. I would expect the tool<br>
>>> to iterate over the domains.<br>
>>><br>
>><br>
>>There was a bug that caused all this behavior, now it should be OK.<br>
>><br>
>>><br>
>>> The messages that are user visible must be marked as translatable. See<br>
>>> the definition of the ERROR() macro to see how to do that.<br>
>>><br>
>>> When the tool is ready, it's going to require a man page and it needs to<br>
>>> be mentioned in <a href="http://sssd.spec.in" target="_blank">sssd.spec.in</a> so it's packaged in the RPMs correctly.<br>
>>><br>
>><br>
>>I made the man page and also put mention is <a href="http://sssd.spec.in" target="_blank">sssd.spec.in</a>.<br>
>><br>
>>><br>
>>> Please ask your thesis mentor if you can keep the copyright assigned to<br>
>>> yourself in the thesis. I vaguely remember I had to submit the copyright<br>
>>> to FIT VUT back in the day :-)<br>
>>><br>
>><br>
>>In progress, mentor is busy and not responding at the moment :)<br>
>><br>
>>> _______________________________________________<br>
>>> sssd-devel mailing list<br>
>>> <a href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a><br>
>>> <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
>>><br>
>><br>
>>Thanks for all your advice and tips. I uploaded new version with new<br>
>>design, hopefully fixed all the bugs and added all the missing<br>
>>functionality. I will welcome all comments to new design or reports of<br>
>>mistakes.<br>
>><br>
>>Thanks<br>
>>David Bambušek<br>
><br>
>That code is much more better, after redesign code is approximately 35%<br>
>shorter and much more readable.<br>
><br>
>In function print_object, you iterate over array of strings (user_attrs),<br>
> foreach attr in user_attrs:<br>
> int_value = get_value_from_hash(hash, attr)<br>
> switch(int_value)<br>
><br>
> .......<br>
> case A_<type>:<br>
> oa->key = g_strdup(SYSDB_<type>);<br>
> ^^^^^^^^^^^^<br>
> this is the same like a "attr"<br>
> ret_value = talloc_strdup(mem_ctx,ldb_msg_find_attr_as_string(msg, SYSDB_<type>, NULL));<br>
> ^^^^^^^^^^^^<br>
> this is also "attr"<br>
> strcpy(final, "Name:\t ");<br>
> ^^^^^^^^^<br>
> human readable version of "attr"<br>
> break;<br>
><br>
> ......<br>
><br>
><br>
>The purpose of hash_table was to reduce very long switch/case. Hash table will<br>
>help you with mapping one type to another and code will be much more simpler.<br>
><br>
>You can transform current hash table <string, int> (SYSDB_<type>, A_<type>)<br>
>to hash table <string, string> (SYSDB_type, "human readable version of SYSDB_<type>"<br>
>This approach should help you to remove switch/case from print_object function.<br>
><br></div></div></blockquote><div>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.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
><br>
>Also function search_object is longer then 250 lines, so you could try to:<br>
> -- split it to two functions search_object, search_object_all<br>
> -- or you could try to reuse function parameter "bool all"<br>
><br>
>LS<br></div></div></blockquote><div>Functions are now split into three, one for certain object, one for all objects and one for domains and subdomains. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
<br>
</div></div>I forgot to mention one important thing in previous email.<br>
<br>
You have used hash table from glib, but glib is optional dependency in sssd.<br>
For example in debian distribution, sssd does not depend on glib(libglib).<br>
You should use hash table from ding-libs(libdhash)<br>
<br>
Fedora distribution have package libdhash-devel and you can find exmaples<br>
in directory /usr/share/doc/libdhash-devel-0.4.3/<br>
<div class="HOEnZb"><div class="h5"><br>
LS<br></div></div></blockquote><div> </div><div>Sorry for that, I used it by mistake, already corrected. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb">
<div class="h5">
<br>
<br>
<br>
_______________________________________________<br>
sssd-devel mailing list<br>
<a href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a><br>
<a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
</div></div></blockquote></div><br></div><div class="gmail_extra">So the newest version is already on my github, with all the issues mentioned above fixed.<br><br></div><div class="gmail_extra">David Bambušek<br></div></div>