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