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(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 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.