On 10/19/2015 12:00 PM, Sumit Bose wrote:
On Fri, Oct 16, 2015 at 01:47:03PM +0200, Pavel Reichl wrote:
>
>
[snip]
> 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).
The check was added to fix
https://fedorahosted.org/sssd/ticket/1202 was
we should disable paging here. I think it makes most sense to do 1)
nevertheless I would add a debug message here because in theory this
case should never happen in the first place because if makes no sense
for a caller to use a base search with paging and hence it should be
fixed on the caller side as well.
We also set allow_paging = true if some controls are to be used without checking the
scope. I added warning for that case (please let me know if it's useless).
Maybe we want to be even more radical in a later release and just return
an error in this case. But I would start with the debug message to see
if there might be some legitimate cases where it is not clear how the
parameters are set and sdap_get_generic_ext_send() is the best place to
sort it out.
>
>>
>>> };
>>>
>>> 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(a)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().
no, please keep them. sdap_get_generic_ext_send() is our wrapper for
ldap_search_ext() and should make all option ldap_search_ext() has
available to callers in SSSD.
Would it make sense to add warnings as I proposed in patch 4?
It looks we only do not set the timeout parameter in ldap_search_ext().
This is ok, since it only controls the server-side timeout and the
timeout is handled separately by the sdap_op scheme. Nevertheless it
might make sense to talk to LDAP server developers to see if it would
help to set the option as well. I'm thinking of cases where SSSD might
trigger a long running operation on the server but drops the request
after a few seconds on the client. The server is then still busy
preparing the response for the client and when it is ready it is not
needed anymore. So sending the timeout might help to reduce the server
load? But of course this should not be part of the current set of
patches and should be handled independently.
I filled
https://fedorahosted.org/sssd/ticket/2843
Thanks for hints. Please see updated patch set attached.
bye,
Sumit
>
> Thanks!
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel