On Mon, Aug 17, 2015 at 10:32:00AM +0200, Jakub Hrozek wrote:
On Sun, Aug 16, 2015 at 05:59:22PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2737
Hi, it should work... :-) However, I wanted to make import as transaction so no changes are made if some error occurs, but I had some troubles with it so I gave up eventually.
Thanks a lot for the patches. I didn't do any review yet, just scrolled through them.
My first reaction was that the second patch should be split up and at least the colondb should be unit tested.
btw did you get inspired by some existing code, like libuser or shadow-utils with the colondb?
The patches are quite big, so maybe the review will be delivered in chunks.
1) There are some Coverity errors: Error: COMPILER_WARNING: sssd-1.13.1/src/tools/common/sss_colondb.c:279:12: warning: 'fmode' may be used uninitialized in this function [-Wmaybe-uninitialized] # fp = fopen(filename, fmode); # ^ # 277| if (filename != NULL) { # 278| errno = 0; # 279|-> fp = fopen(filename, fmode); # 280| if (fp == NULL) { # 281| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to open file %s\n", filename);
Error: RESOURCE_LEAK (CWE-772): sssd-1.13.1/src/tools/common/sss_colondb.c:279: alloc_fn: Storage is returned from allocation function "fopen". sssd-1.13.1/src/tools/common/sss_colondb.c:279: var_assign: Assigning: "fp" = storage returned from "fopen(filename, fmode)". sssd-1.13.1/src/tools/common/sss_colondb.c:289: leaked_storage: Variable "fp" going out of scope leaks the storage it points to. # 287| if (db == NULL) { # 288| DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero() failed\n"); # 289|-> return NULL; # 290| } # 291|
Error: COMPILER_WARNING: sssd-1.13.1/src/tools/common/sss_colondb.c: scope_hint: In function 'sss_colondb_open' sssd-1.13.1/src/tools/common/sss_colondb.c:292:14: warning: 'fp' may be used uninitialized in this function [-Wmaybe-uninitialized] # db->file = fp; # ^ # 290| } # 291| # 292|-> db->file = fp; # 293| db->mode = mode; # 294|
Error: UNINIT (CWE-457): sssd-1.13.1/src/tools/sss_override.c:997: var_decl: Declaring variable "tmp_ctx" without initializer. sssd-1.13.1/src/tools/sss_override.c:1076: uninit_use_in_call: Using uninitialized value "tmp_ctx" when calling "_talloc_free". # 1074| # 1075| done: # 1076|-> talloc_free(tmp_ctx); # 1077| return exit; # 1078| }
Error: UNINIT (CWE-457): sssd-1.13.1/src/tools/sss_override.c:1207: var_decl: Declaring variable "tmp_ctx" without initializer. sssd-1.13.1/src/tools/sss_override.c:1282: uninit_use_in_call: Using uninitialized value "tmp_ctx" when calling "_talloc_free". # 1280| # 1281| done: # 1282|-> talloc_free(tmp_ctx); # 1283| return exit; # 1284| }
2) There is no documentation. It would be nice to have some examples in the man page also.
3) I should probably test this before asking, but do the import and export commands support blank (skipped) fields?