[SSSD] [PATCHES] SDAP: refactor sdap_access

Pavel Březina pbrezina at redhat.com
Thu Aug 14 10:09:54 UTC 2014


On 08/13/2014 05:06 PM, Pavel Reichl wrote:
>
> On 08/13/2014 02:32 PM, Pavel Březina wrote:
>> On 08/13/2014 10:49 AM, Pavel Reichl wrote:
>>>
>>> On 08/12/2014 02:16 PM, Pavel Březina wrote:
>>>> On 08/11/2014 05:55 PM, Pavel Reichl wrote:
>>>>> Hello,
>>>>>
>>>>> I was struggling with rebasing my patches in the thread "SDAP: Be able
>>>>> to configure sssd to honor openldap account lock to restrict access
>>>>> via
>>>>> ssh key" so I decided to split the patch set into smaller chunks to be
>>>>> able to quickly react on review comments and not to fight with
>>>>> rebasing
>>>>> all the time.
>>>>>
>>>>> Attached patches prepare code for implementing #2364
>>>>>
>>>>> Patch:
>>>>> 1st) acked by Pavel
>>>>> 2nd) Jakub requsted change:
>>>>>> +static errno_t sdap_get_basedn_user_entry(TALLOC_CTX *mem_ctx,
>>>>>> +                                          struct ldb_message
>>>>>> *user_entry,
>>>>>> +                                          const char *username,
>>>>>> +                                          char **_basedn)
>>>>>> +{
>>>>>> +    const char *basedn;
>>>>>> +    errno_t ret;
>>>>>> +
>>>>>> +    basedn = ldb_msg_find_attr_as_string(user_entry, SYSDB_ORIG_DN,
>>>>>> NULL);
>>>>>> +    if (basedn == NULL) {
>>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,"Could not find originalDN for
>>>>>> user
>>>>>> [%s]\n",
>>>>>> +              username);
>>>>>> +        ret = EINVAL;
>>>>>> +        goto done;
>>>>>> +    }
>>>>>> +
>>>>>> +    *_basedn = talloc_strdup(mem_ctx, basedn);
>>>>>> +    if (*_basedn == NULL) {
>>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
>>>>>> +              "Could not allocate memory for originalDN\n");
>>>>>> +        ret = ENOMEM;
>>>>>> +        goto done;
>>>>>> +    }
>>>>>> Do we need to strdup the basedn here?
>>>>>>
>>>>> 3rd)  acked by Pavel
>>>>> 4th) Jakub comment:
>>>>>
>>>>>> This function is misplaced, it immediatelly follows _send() in the
>>>>>> code.
>>>>>> _recv() should be the last in the flow.
>>>>>
>>>>> 5th) acked by Pavel
>>>>>
>>>>> I hope attached patches address Jakub's concerns.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Pavel Reichl
>>>>
>>>> Patch 1: Ack.
>>>> Patch 2: Ack. Just the commit message needs some line wrapping.
>>>> Patch 3: Ack.
>>>> Patch 4:
>>>> It is good to order function by its execution time when you work in
>>>> tevent. I.e. the correct order should be:
>>>> _send
>>>> check_next_rule (which would be nice to also rename to
>>>> sdap_access_next_rule or similar)
>>>> _filter_done
>>>> _done
>>>> _recv
>>>
>>> OK, I renamed the function and updated the function definition order as
>>> you proposed.
>>>
>>> But I'm wondering whether according to the rule you described
>>> sdap_access_filter_send should  immediately follow check_next_rule as it
>>> is called before sdap_access_filter_done?
>>
>> Hi,
>> this is unfortunately cause of poor naming. Thanks for pointing it
>> out, we will fix that now :-)
>>
>> What do we have now are two tevent requests:
>> 1) sdap_access_send
>>    - sdap_access_check_next_rule
>>    - sdap_access_filter_done
>>    - sdap_access_lock_done (future patch)
>>    - sdap_access_done
>>    - sdap_access_subreq_recv
>>    - sdap_access_recv
>>
>> sdap_access_check_next_rule can either:
>> a) create a new subrequest for filter check, setting
>> sdap_access_filter_done as callback
>> b) same as a but for lock in future patches
>> c) finish the request 1
>>
>> 2) sdap_access_filter_send
>>    - sdap_access_filter_retry
>>    - sdap_access_filter_connect_done
>>    - sdap_access_filter_get_access_done
>>    - sdap_access_filter_recv
>>
>> It is hard to follow such code. We should improve it this way:
> Hmm I personally don't think this is much an improvement.  Anyway, I
> (again) updated patches as you instructed, I hope I did it as you wished
> and that you can finally (again) ACK the patches.

You just forgot to do one change - move sdap_access_filter_recv after 
sdap_access_filter_done.

And I just notice one more thing - you access state of 
sdap_access_filter outside of the request - that is bad.

- move sdap_access_filter_req_ctx before sdap_access_filter_send. State 
structures should be named $prefix_state, this is an old code so I don't 
require you to rename it now, since the whole file uses this 
$prefix_req_ctx notation, but remember that when you create new tevent 
request.

- ac_type field you created should be part of sdap_access_req_ctx

>>
>> 1) sdap_access_send
>>    - sdap_access_check_next_rule
>>    - sdap_access_done
>>    - sdap_access_recv
>>
>> We will remove sdap_access_{filter|lock}_done, they only call
>> sdap_access_done anyway. We will move code from
>> sdap_access_subreq_recv to sdap_access_done directly. There is no code
>> duplication anyway so you don't have to be worried about it.
>>
>> 2) sdap_access_filter_send
>>    - sdap_access_filter_retry
>>    - sdap_access_filter_connect_done
>>    - sdap_access_filter_done
>>    - sdap_access_filter_recv
>>
>> We will rename sdap_access_filter_get_access_done to
>> sdap_access_filter_done.
>>
>> Tevent guideline is to start each request with $prefix_send, continue
>> with $prefix_$part_step or $prefix_$part_done, end it with
>> $prefix_done and than allow the caller to fetch result with
>> $prefix_recv. The code should be read from top to bottom without
>> interruption.
>>
>>>> Patch 5: Ack.




More information about the sssd-devel mailing list