[SSSD] [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Pavel Reichl preichl at redhat.com
Fri Oct 16 11:47:03 UTC 2015



On 10/15/2015 11:24 AM, Sumit Bose wrote:
> On Wed, Oct 14, 2015 at 04:09:25PM +0200, Pavel Reichl wrote:
>>
>>
>> On 10/14/2015 01:07 PM, Pavel Reichl wrote:
>>>
>>>
>>> On 10/14/2015 01:01 PM, Jakub Hrozek wrote:
>>>> On Wed, Oct 14, 2015 at 12:51:39PM +0200, Sumit Bose wrote:
>> [snip]
>>>>> So far I liked the flags attribute which controls the behaviour of
>>>>> sdap_get_generic_ext_send() best (and I agree that allow_paging should
>>>>> be include in the flags, but only for sdap_get_generic_ext_send() not
>>>>> the "higher" level calls). Mainly because it scales, i.e. it can be used
>>>>> in future to control sdap_get_generic_ext_send() even more.
>>>>>
>> [snip]
>>>>> If you think this is all too much for the issue at hand (ignoring a
>>>>> debug message) especially for versions that are already released what
>>>>> about always ignoring the message (or only displaying with
>>>>> SSSDBG_TRACE_ALL) if state->sizelimit != 0, i.e. explicitly set?
>>>>
>>>> No, I don't really mind, I think we already spent too much time
>>>> discussing this simple patch.
>>>>
>>>> If you like the flags best, let's push the flags..
>>>> _______________________________________________
>>>
>>> OK, I'll send updated patch reflecting comments relating 'flags' patch.
>>> _______________________________________________
>>
>> Please see updated patch set.
>> 1st patch adds flag to control silencing of warning
>> 2nd patch makes allow_paging part of flag
>> 3rd patch removes unused parameter 'attrsonly' from function sdap_get_and_parse_generic_send()
>> 4rd patch makes attrsonly in sdap_get_generic_ext_state to be part of flag
>>
>> If any of patches 2,3,4 is too controversial feel free to ignore it, I can send it later in separate thread.
>>
>> Thanks!
>
> Thank you for the patches.
>
> I have two things I would like to discuss
>
>> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
>> index b81431f79f21755469bb9ff123d695a2a166e353..cbf199e6d524a3aa659ef040937b26571ab63735 100644
>> --- a/src/providers/ldap/sdap_async.c
>> +++ b/src/providers/ldap/sdap_async.c
>> @@ -1164,6 +1164,7 @@ struct sdap_get_generic_ext_state {
>>       void *cb_data;
>>
>>       bool allow_paging;
>> +    bool sizelimit_silent;
>
> What about putting the flags directly in the state and check them only
> where they might apply. I think this will scale better because there is
> no need to add a boolean for every new flag value and does not need the
> (mostly useless?) conversion from flag to bool at the start of
> sdap_get_generic_ext_send().

Yes, that was my idea too. Then while working on second patch I noticed that allow_paging is not set only by value of allow_paging argument but depends on scope as well.

     if (scope == LDAP_SCOPE_BASE) {
         state->allow_paging = false;
     } else {
         state->allow_paging = flags & SDAP_SRCH_FLG_PAGING;
     }

I'm not sure how to handle this:
1) remove boolean (allow_paging, sizelimit_silent, ...) fields from state altogether and use flags field for writing and reading.


	- if (scope == LDAP_SCOPE_BASE) {
	-	state->allow_paging = false;
	- } else {
	-	state->allow_paging = flags & SDAP_SRCH_FLG_PAGING;
	- }
	+ if (scope == LDAP_SCOPE_BASE) {
	+ 	state->flags &= ~SDAP_SRCH_FLG_PAGING
	+ }
2) keep boolean field for allow_paging only?
3) keep boolean fields for all values set by flags parameter and do not store flags as a field in state.

1) and 3) is fine by me, I don't like 2) because it's slightly inconsistent in the sense that allow_paging can be read from flags and has its own boolean field (both might have a different value).

>
>>   };
>>
>>   static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
>> @@ -1172,6 +1173,9 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
>>                                            struct sdap_msg *reply,
>>                                            int error, void *pvt);
>>
>> +/* Be silent about exceeded size limit */
>> +#define SDAP_SRCH_FLG_SIZELIMIT_SILENT    0x01
>
> I wonder if the (1 << 0) style notation used e.g. for flags in the
> pam_sss module or for SBUS_PROPERTY_* would be less error-prone to
> define flag values?

Thanks for the idea, I'll add this change to the patch set.

>
> Otherwise the patches look good and I think it is a good idea to include
> attrsonly here as well.
>
> I didn't run any tests so far because I would like to hear your option
> about the two suggestions from above first.
>
> bye,
> Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

Sumit what do you think about patch 3? I noticed that also **serverctrls and **clientctrls are unused, should I remove all 3 parematers or should I pass them instead? I checked the code and there would be no difference in the actual parameters passed 
to sdap_get_generic_ext_send().

Thanks!


More information about the sssd-devel mailing list