Hi Jakub,

I have made necessary changes, please review it.


On Fri, Apr 5, 2013 at 4:09 AM, Jakub Hrozek <jhrozek@redhat.com> wrote:
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:

Feel free to ping me on #sssd if you need any help.
sssd-devel mailing list