On 08/11/2016 08:53 AM, Petr Cech wrote:
> On 08/03/2016 12:34 PM, Michal Židek wrote:
>> Two nitpicks, see inline.
>>
>> On 07/22/2016 02:34 PM, Petr Cech wrote:
>>>
>>> +static errno_t add_to_missing_attrs (TALLOC_CTX * mem_ctx,
>>> + struct sysdb_attrs *attrs,
>>> + const char *ext_key,
>>> + char ***_missing)
>> ^
>> Coding style. Remove the space between function name and "(".
>> Do not forget to align the parameters after that.
>
> Addressed.
>
>>> +{
>>> + bool is_present = false;
>>> + size_t size = 0;
>>> + size_t ret;
>>> +
>>> + for (int i = 0; i < attrs->num; i++) {
>>> + if (strcmp(ext_key, attrs->a[i].name) == 0) {
>>> + is_present = true;
>>> + }
>>> + size++;
>>> + }
>>> +
>>> + if (is_present == false) {
>>> + ret = add_string_to_list(attrs, ext_key, _missing);
>>> + if (ret != EOK) {
>>> + goto fail;
>>> + }
>>> + }
>>> +
>>> + ret = EOK;
>>> +
>>> +fail:
>>
>> Please change the label name to "done". According to
>> our new coding style, the code that follows label "fail"
>> is only executed when failure occurs. I know we do not
>> follow this everywhere, but I would like to be consistent
>> in new code.
>
> Addressed.
>
>>> + return ret;
>>> +}
>>> +
>>
>> Other than that it looks good to me.
>>
>> Also it would be good to add a CI tests for this. I do
>> not want to postpone this patch before release, so you can
>> do it later as part of this ticket:
>>
https://fedorahosted.org/sssd/ticket/3119
>>
>> So either send a patch with CI test now or
>> assign the above ticket to yourself and do it
>> when there is more time.
>>
>> Michal
>
> Hello,
>
> there is fixed patch attached.
>
> I am working on CI test. I hope it will be ready soon.
>
> Regards.
CI INTG tests will be solved in another mail thread. They are dependent
on Lukas patches.
Tests are solved in
[SSSD] [PATCH SET] WIP: INTG: Tests for ldap nested netgroups
mainling thread.
--
Petr^4 Čech