Adding sssd-devel list back to CC
Dne čtvrtek 26 července 2012 18:14:40, Michal Zidek napsal(a):
On 07/26/2012 02:25 PM, Jan Zelený wrote:
> Dne středa 25 července 2012 13:21:01, Michal Zidek napsal(a):
>> Unit test for src/db/sysdb_ssh.c
>
> Nack,
> you are cleaning the directory TESTS_PATH but what if the directory
> existed
> (and maybe wasn't empty) before? The least you should do is to exclude the
> return code from ret variable.
The TESTS_PATH directory is created only for this unit test needs (it is
actually tests_sysdb_ssh
directory). If this directory existed before (unlikely, but possible)
and contains files other than
tests_conf.ldb and sssd.ldb, then those will remain untouched (rmdir
syscall only removes empty
directories). However, if the file existed before and contained files
with the same names, then these
will be overwritten, but I don't think this is a big problem because
such files are probably only garbage
from previous unfinished tests where the clean_up function was not
called (because of segfault or
something). And unlink and rmdir only returns 0 or -1, so I think it is
OK to test them the way I did (it
saves some lines of code).
Ok, I have no big problem with this.
> Please use the SSSD_MAIN_OPTS macro in the variable long_options
instead
> defining the debug-level manually there?
>
> An important code-style comment: please declare all variables at the
> beginning of functions. There are only a handful exceptions of this rule
> in the code.
>
> Nitpick: you are missing space before SYSDB_OBJECTCLASS in
> store_one_host_test()
>
> Consider checking results of all functions (for example function like
> talloc_strdup(), talloc_zero(), sysdb_attrs_add_string(), ...) or at least
> those on which something else is depending (like allocation of structures)
> so we don't get segfaults. I'm not stating this as a hard requirement
> sice we don't have it in most of places in sysdb test suite, but it would
> be nice to make the coding style a little bit more defensive.
>
> In delete_nonexisting_host_test():
> nonexisting -> nonexistent
> Deleting -> Deletion
>
>
> Other than that the patch is ok.
>
> Thanks
> Jan
Thanks. New patch attached.
Just two more minor things:
src/tests/sysdb_ssh-tests.c: In function ‘main’:
src/tests/sysdb_ssh-tests.c:409:9: warning: unused variable ‘debug’ [-Wunused-
variable]
Bad indentation level on line 285 of the patch.
Thanks
Jan