<div dir="ltr"><div><div>Hi Jakub,<br><br></div>I have made necessary changes, please review it.<br><br></div>thanks,<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Apr 5, 2013 at 4:09 AM, Jakub Hrozek <span dir="ltr">&lt;<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Thu, Apr 04, 2013 at 02:44:47AM +0530, Abhishek Singh wrote:<br>
&gt; Hi Jakub,<br>
&gt;<br>
&gt; I have done requested changes and have attached the patch with this mail.<br>
&gt; Kindly review it.<br>
&gt;<br>
&gt; thanks,<br>
&gt;<br>
&gt;<br>
<br>
</div>Hi Abhishek,<br>
<br>
this is much better and we&#39;re almost there. I only have a couple of<br>
comments before I push the patch:<br>
<br>
1) I couldn&#39;t apply the patch cleanly. Can you make sure that the<br>
patches you send to the list can be applied on top of origin/master? In<br>
most cases, all you have to do is continuously rebase using &quot;git rebase<br>
origin/master&quot; and fix the conflicts.<br>
<br>
If there are any patches that needs to be applied along with this one,<br>
feel free to send them along in the same e-mail as another attachment.<br>
<br>
2) because I couldn&#39;t apply the patch I only did visual review. I have<br>
one nitpick:<br>
<br>
+static char *get_filepath(char path[23])<br>
+{<br>
+    int ret;<br>
+<br>
+    strncpy(path, FILE_PATH, 23);<br>
<br>
I&#39;m not fond of the magic constant &quot;23&quot; used here.  I think it would be<br>
better to either declare that path is always PATH_MAX long or pass its<br>
size as additional parameter and declare path as &quot;char *&quot;.<br>
<br>
3) The get_dirfd() function assigns a DIR pointer but never calls<br>
closedir() to free this resource. It&#39;s not a big deal in tests, but we<br>
should still try to make the code as good as possible.<br>
<br>
I think in this case actually a &quot;fixture&quot; might be a good idea. A fixture<br>
in unit tests is a function that gets run before the test is executed<br>
and another after the test is done. I think for this test it might be a<br>
good idea to include a setup fixture to open the directory with opendir()<br>
and a teardown fixture to close the handle with closedir().<br>
<br>
Feel free to use either a global variable or the **state pointer for<br>
storing the dir handle, both would work for me. If you need more<br>
information on using the cmocka fixtures, some documentation is<br>
available at:<br>
<a href="https://cmockery.googlecode.com/svn/trunk/doc/index.html#Test_State" target="_blank">https://cmockery.googlecode.com/svn/trunk/doc/index.html#Test_State</a><br>
<br>
Feel free to ping me on #sssd if you need any help.<br>
<div class="HOEnZb"><div class="h5">_______________________________________________<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>