On 08/30/2016 01:31 PM, Pavel Březina wrote:
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.
Hello,
Pavel, thanks for reminder, I solve similar case for groups now.
Please, see attached patch.
I removed DEBUG if sdap_get_user_primary_name() failes because it
is not important from global point of view. And there is no goto :-)
I hope this little improvement will be helpful for all admins.
Regards
--
Petr^4 Čech