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.