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