On Thu, Aug 4, 2016 at 8:08 AM, Petr Cech <pcech(a)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 (usn_value) {
> --
>
> Reviewed-by: Fabiano Fidêncio <fidencio(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
Best Regards,
--
Fabiano Fidêncio