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

Sumit Bose sbose at redhat.com
Thu Oct 15 09:24:59 UTC 2015


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().

>  };
>  
>  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?

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


More information about the sssd-devel mailing list