[SSSD] [PATCHES] SDAP: refactor sdap_access
Pavel Reichl
preichl at redhat.com
Wed Aug 13 15:06:24 UTC 2014
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.
>
> 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.
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SDAP-split-sdap_access_filter_get_access_done.patch
Type: text/x-patch
Size: 3813 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140813/f81b1a20/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SDAP-refactor-sdap_access_filter_send.patch
Type: text/x-patch
Size: 3563 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140813/f81b1a20/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-SDAP-nitpicks-in-sdap_access_filter_get_access_done.patch
Type: text/x-patch
Size: 2137 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140813/f81b1a20/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-SDAP-refactor-sdap_access_filter_done.patch
Type: text/x-patch
Size: 7502 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140813/f81b1a20/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-SDAP-don-t-log-error-on-access-denied.patch
Type: text/x-patch
Size: 1149 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140813/f81b1a20/attachment-0004.bin>
More information about the sssd-devel
mailing list