On 08/30/2016 01:29 PM, Pavel Březina wrote:
On 08/04/2016 08:28 AM, Fabiano Fidêncio wrote:
On Thu, Aug 4, 2016 at 8:08 AM, Petr Cech pcech@redhat.com wrote:
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.
Why not? :-) You set user_name to NULL in the beginning of the function and it is just set again when sdap_get_primary_fqdn() is successful. Checking whether _output_arg is NULL or not seems sane in this case.
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).
I'll check it out as well :-)
If this function fails, we may as well quit processing this user and continue with the next one, since if it doesn't have a name, it is an invalid record and we can't store it (it fails in subsequent call of sdap_save_user).
BTW This is what we usually do when we can't get a name for debug message in a cycle.