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?