[SSSD] [PATCHES] SDAP: refactor sdap_access

Pavel Březina pbrezina at redhat.com
Thu Aug 14 13:35:30 UTC 2014


On 08/14/2014 02:16 PM, Pavel Reichl wrote:
>
> On 08/14/2014 12:09 PM, Pavel Březina wrote:
>> 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.

Hi, you forgot to move the state structure to the original position. I'm 
sending a patch to squash. Otherwise it's an ack.

>>
>> - ac_type field you created should be part of sdap_access_req_ctx
>>
> OK, thanks for noticing and explanation!
>>>>
>>>> 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
>
>
>
> _______________________________________________
> 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-squash-to-SDAP-refactor-sdap_access_filter_done.patch
Type: text/x-patch
Size: 2038 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140814/278d7413/attachment.bin>


More information about the sssd-devel mailing list