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

Sumit Bose sbose at redhat.com
Mon Oct 19 10:00:42 UTC 2015


On Fri, Oct 16, 2015 at 01:47:03PM +0200, Pavel Reichl wrote:
> 
> 
> 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).

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. 

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

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. 

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.

bye,
Sumit

> 
> Thanks!
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


More information about the sssd-devel mailing list