6:47 a.m.
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(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().
Thanks!