[SSSD] [PATCHES] SDAP: refactor sdap_access

Pavel Březina pbrezina at redhat.com
Wed Aug 13 12:32:16 UTC 2014


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:

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