On Thu, Aug 4, 2016 at 8:08 AM, Petr Cech <pc...@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 <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
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
