[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