On 08/03/2016 09:54 AM, Fabiano Fidêncio wrote:
Hey!
I'd do it a bit differently.
Hello Fabiano,
I am glad for another point of view.
diff --git a/src/providers/ldap/sdap_async_users.c
b/src/providers/ldap/sdap_async_users.c
index
e44c045b3f8ff6aed33a42cf2919bc01aa41a243..3a8efa4caacbad74f493de334a387104d0e7cec4
100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -519,6 +519,7 @@ int sdap_save_users(TALLOC_CTX *memctx,
char **_usn_value)
{
TALLOC_CTX *tmpctx;
+ const char* user_name = NULL;
^-----
I know this should be 'const char *'. I will fix it.
char *higher_usn = NULL;
char *usn_value;
int ret;
@@ -548,14 +549,22 @@ int sdap_save_users(TALLOC_CTX *memctx,
for (i = 0; i < num_users; i++) {
usn_value = NULL;
+ ret = sdap_get_user_primary_name(memctx, opts, users[i],
+ dom, &user_name);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to get user name\n");
+ goto done;
+ }
+
IMO, if it fails, that's okay, then let's just go for using the loop index.
IOW, please, remove the "goto done;" line.
Right, I will fix it.
ret = sdap_save_user(tmpctx, opts, dom, users[i], &usn_value, now);
/* Do not fail completely on errors.
* Just report the failure to save and go on */
if (ret) {
- DEBUG(SSSDBG_OP_FAILURE, "Failed to store user %d.
Ignoring.\n", i);
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to store user %s. Ignoring.\n",
+ user_name);
} else {
- DEBUG(SSSDBG_TRACE_ALL, "User %d processed!\n", i);
+ DEBUG(SSSDBG_TRACE_ALL, "User %s processed!\n", user_name);
}
I didn't detailed check what sdap_get_user_primary_name() is doing,
but I have the feeling that when it fails, user_name is NULL.
So, when printing the debug message, you can check whether the
username is not NULL and print it, otherwise you could print the
array's index as it was done before your patch.
I did some investigation.
If error occurs in function sdap_get_user_primary_name(), the function
doesn't touch return argument (here it is &user_name), so there needn't
be a NULL value in user_name.
I discussed this topic with Lukas offline. Our practise is:
int function(input_arg, ..., _output_arg)
if (ret != 0) than there is no guarantee that _output_arg makes sense.
In our case, _output_arg isn't NULL. It's a pitty.
There are some comments about using functions such
sdap_get_user_primary_name() in thread 'LDAP: Do not print "null" in the
DEBUG message' (I need read this, thanks Lukas for info).
if (usn_value) {
--
Reviewed-by: Fabiano Fidêncio <fiden...@redhat.com>
Best Regards,
This mail is just note and comments. I will work on it later.
Regards
--
Petr^4 Čech
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
