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

[Jakub Hrozek]

On Tue, Oct 20, 2015 at 03:54:51PM +0200, Sumit Bose wrote:
> On Mon, Oct 19, 2015 at 06:49:21PM +0200, Pavel Reichl wrote:
> > 
> > 
> > On 10/19/2015 05:56 PM, Sumit Bose wrote:
> > >On Mon, Oct 19, 2015 at 05:01:50PM +0200, Pavel Reichl wrote:
> > >
> > [snip]
> > >>
> > >>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).
> > >
> > >I'm afraid they are. The deref/asq controls typically have the based
> > >scope (iirc for asq it is even required).
> > >
> > 
> > Fixed, thanks.
> > [snip]
> > >>
> > >>Would it make sense to add warnings as I proposed in patch 4?
> > >
> > >oh. Sorry, I didn't checked the current code more carefully before. It
> > >does not make sense to call sdap_get_generic_ext_send() from
> > >sdap_get_and_parse_generic_send() with dropping attrsonly, serverctrls
> > >and clientctrls. Please instead of writing a warning just pass them down
> > >to sdap_get_generic_ext_send().
> > >
> > No problem, I should have been able to describe the problem without need to 
> > look into code.
> > 
> > [snip]
> > >>  /* Try to return what we've got */
> > >>-DEBUG(SSSDBG_MINOR_FAILURE,
> > >>-  "LDAP sizelimit was exceeded, returning incomplete 
> > >>data\n");
> > >>+
> > >>+bool sizelimit_silent =
> > >>+state->flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT;
> > >>+
> > >>+if (sizelimit_silent == false) {
> > >
> > >I think there is no need for an extra variable and
> > >
> > >if ( ! (state->flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT)) {
> > >
> > >is imo quite readable as well.
> > >
> > OK, fixed.
> > 
> > Please see updated attached patch set.
> 
> Thank you, I do not have any further comments and the patches pass CI as
> well http://sssd-ci.duckdns.org/logs/job/30/83/summary.html, so ACK.
> 
> Please note that the 1st patch only supress the "LDAP sizelimit was
> exceeded, returning incomplete data" at debug level
> SSSDBG_MINOR_FAILURE. But the general mesages about return code at level
> SSSDBG_TRACE_FUNC will be still available:
> 
> (Tue Oct 20 13:49:51 2015) [sssd[be[ad.devel]]] 
> [sdap_get_generic_op_finished] (0x0400): Search result: Size limit 
> exceeded(4), no errmsg set
> 
> bye,
> Sumit

Sorry for the delay in pushing:

master:
86ffb3db2a6e798aaa920a0b40e0c517db8c293f
108af0012e016b464790478b8aa3ad60e712930f
45363a04548738ac99a5d173e3fe021c28b61aec
1f1b41931d299d6356ac205b75b402adb2cc9234 
sssd-1-13:
86ec0fddae2c6f3a39a51034e920d1b3ee1344d8
a46599bab436701081e4596a17c31b51cfd2897f
99ac13e266773703c54a4796e8f36561a3c26054
fed99d90d2c85f2322ed68dec7f8ba8bbcc5062a 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Sumit Bose]

On Mon, Oct 19, 2015 at 06:49:21PM +0200, Pavel Reichl wrote:
> 
> 
> On 10/19/2015 05:56 PM, Sumit Bose wrote:
> >On Mon, Oct 19, 2015 at 05:01:50PM +0200, Pavel Reichl wrote:
> >
> [snip]
> >>
> >>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).
> >
> >I'm afraid they are. The deref/asq controls typically have the based
> >scope (iirc for asq it is even required).
> >
> 
> Fixed, thanks.
> [snip]
> >>
> >>Would it make sense to add warnings as I proposed in patch 4?
> >
> >oh. Sorry, I didn't checked the current code more carefully before. It
> >does not make sense to call sdap_get_generic_ext_send() from
> >sdap_get_and_parse_generic_send() with dropping attrsonly, serverctrls
> >and clientctrls. Please instead of writing a warning just pass them down
> >to sdap_get_generic_ext_send().
> >
> No problem, I should have been able to describe the problem without need to 
> look into code.
> 
> [snip]
> >>  /* Try to return what we've got */
> >>-DEBUG(SSSDBG_MINOR_FAILURE,
> >>-  "LDAP sizelimit was exceeded, returning incomplete 
> >>data\n");
> >>+
> >>+bool sizelimit_silent =
> >>+state->flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT;
> >>+
> >>+if (sizelimit_silent == false) {
> >
> >I think there is no need for an extra variable and
> >
> >if ( ! (state->flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT)) {
> >
> >is imo quite readable as well.
> >
> OK, fixed.
> 
> Please see updated attached patch set.

Thank you, I do not have any further comments and the patches pass CI as
well http://sssd-ci.duckdns.org/logs/job/30/83/summary.html, so ACK.

Please note that the 1st patch only supress the "LDAP sizelimit was
exceeded, returning incomplete data" at debug level
SSSDBG_MINOR_FAILURE. But the general mesages about return code at level
SSSDBG_TRACE_FUNC will be still available:

(Tue Oct 20 13:49:51 2015) [sssd[be[ad.devel]]] [sdap_get_generic_op_finished] 
(0x0400): Search result: Size limit exceeded(4), no errmsg set

bye,
Sumit
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Pavel Reichl]

On 10/20/2015 03:54 PM, Sumit Bose wrote:
ease see updated attached patch set.


Thank you, I do not have any further comments and the patches pass CI as
well http://sssd-ci.duckdns.org/logs/job/30/83/summary.html, so ACK.

Please note that the 1st patch only supress the "LDAP sizelimit was
exceeded, returning incomplete data" at debug level
SSSDBG_MINOR_FAILURE. But the general mesages about return code at level
SSSDBG_TRACE_FUNC will be still available:

(Tue Oct 20 13:49:51 2015) [sssd[be[ad.devel]]] [sdap_get_generic_op_finished] 
(0x0400): Search result: Size limit exceeded(4), no errmsg set

bye,
Sumit



Yes, thanks for testing.

I wrote about that in one of the first mails but it got lost in history:


BTW:
Before the patch logs contained:
[sdap_get_generic_op_finished] (0x0080): LDAP sizelimit was exceeded, returning 
incomplete data
[sdap_get_generic_op_finished] (0x0400): Search result: Size limit exceeded(4), 
no errmsg set

after the patch it contains only:
[sdap_get_generic_op_finished] (0x0400): Search result: Size limit exceeded(4), 
no errmsg set






___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Sumit Bose]

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 

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

[Pavel Reichl]

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_SILENT0x01


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@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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

>From 4024dd2f9c3e835f18ec044773ea6539eae7c421 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH 1/4] SDAP: optional warning - sizelimit exceeded in POSIX
 check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
 src/providers/ldap/sdap_async.c | 34 +-
 1 

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

[Sumit Bose]

On Mon, Oct 19, 2015 at 05:01:50PM +0200, Pavel Reichl wrote:
> 
> 
> 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).

I'm afraid they are. The deref/asq controls typically have the based
scope (iirc for asq it is even required). 

> 
> >
> >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_SILENT0x01
> >>>
> >>>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@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?

oh. Sorry, I didn't checked the current code more carefully before. It
does not make sense to call sdap_get_generic_ext_send() from
sdap_get_and_parse_generic_send() with dropping attrsonly, serverctrls
and clientctrls. Please instead of writing a warning just pass them down
to sdap_get_generic_ext_send().

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

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

[Pavel Reichl]

On 10/19/2015 05:56 PM, Sumit Bose wrote:

On Mon, Oct 19, 2015 at 05:01:50PM +0200, Pavel Reichl wrote:


[snip]


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


I'm afraid they are. The deref/asq controls typically have the based
scope (iirc for asq it is even required).



Fixed, thanks.
[snip]


Would it make sense to add warnings as I proposed in patch 4?


oh. Sorry, I didn't checked the current code more carefully before. It
does not make sense to call sdap_get_generic_ext_send() from
sdap_get_and_parse_generic_send() with dropping attrsonly, serverctrls
and clientctrls. Please instead of writing a warning just pass them down
to sdap_get_generic_ext_send().


No problem, I should have been able to describe the problem without need to 
look into code.

[snip]

  /* Try to return what we've got */
-DEBUG(SSSDBG_MINOR_FAILURE,
-  "LDAP sizelimit was exceeded, returning incomplete data\n");
+
+bool sizelimit_silent =
+state->flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT;
+
+if (sizelimit_silent == false) {


I think there is no need for an extra variable and

if ( ! (state->flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT)) {

is imo quite readable as well.


OK, fixed.

Please see updated attached patch set.
>From 004c3397b93ffc5693ef1614ec98e5a5af6aaa8c Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH 1/4] SDAP: optional warning - sizelimit exceeded in POSIX
 check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
 src/providers/ldap/sdap_async.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index b81431f79f21755469bb9ff123d695a2a166e353..2efc901ef2bfb07a531aaf506737c94fa6c82bd9 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;
+unsigned int flags;
 };
 
 static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,6 +1173,11 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
  struct sdap_msg *reply,
  int error, void *pvt);
 
+enum {
+/* Be silent about exceeded size limit */
+SDAP_SRCH_FLG_SIZELIMIT_SILENT = 1 << 0,
+};
+
 static struct tevent_req *
 sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   struct tevent_context *ev,
@@ -1188,7 +1194,8 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   int timeout,
   bool allow_paging,
   sdap_parse_cb parse_cb,
-  void *cb_data)
+  void *cb_data,
+  unsigned int flags)
 {
 errno_t ret;
 struct sdap_get_generic_ext_state *state;
@@ -1215,6 +1222,7 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
 state->parse_cb = parse_cb;
 state->cb_data = cb_data;
 state->clientctrls = clientctrls;
+state->flags = flags;
 
 if (state->sh == NULL || state->sh->ldap == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE,
@@ -1500,8 +1508,12 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
 
 if (result == LDAP_SIZELIMIT_EXCEEDED) {
 /* Try to return what we've got */
-DEBUG(SSSDBG_MINOR_FAILURE,
-  "LDAP sizelimit was exceeded, returning incomplete data\n");
+
+if ( ! (state->flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT)) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  "LDAP sizelimit was exceeded, "
+  "returning incomplete data\n");
+}
 } else if (result == LDAP_INAPPROPRIATE_MATCHING) {
 /* This error should only occur when we're testing for
  * specialized functionality like the ldap matching rule
@@ -1708,7 +1720,8 @@ struct tevent_req *sdap_get_and_parse_generic_send(TALLOC_CTX *memctx,
 subreq = sdap_get_generic_ext_send(state, ev, opts, sh, search_base,
scope, filter, attrs, false, NULL,
NULL, sizelimit, timeout, allow_paging,
-   sdap_get_and_parse_generic_parse_entry, state);
+   sdap_get_and_parse_generic_parse_entry,
+   state, 0);
 if (!subreq) {
 talloc_zfree(req);
 return NULL;
@@ -1919,7 +1932,7 @@ sdap_x_deref_search_send(TALLOC_CTX *memctx, struct 

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

[Pavel Reichl]

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_SILENT0x01


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@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!
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Sumit Bose]

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_SILENT0x01

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
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Sumit Bose]

On Tue, Oct 06, 2015 at 11:21:54AM +0200, Jakub Hrozek wrote:
> 
> I personally don't care about unsigned vs unsigned int, but see my
> comment about the request inline..
> 
> > From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Fri, 25 Sep 2015 07:05:30 -0400
> > Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
> > 
> > Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
> > be set to suppress warning about 'sizelimit exceeded'.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2804
> > ---
> >  src/providers/ldap/sdap_async.c | 78 
> > +
> >  1 file changed, 56 insertions(+), 22 deletions(-)
> > 
> > diff --git a/src/providers/ldap/sdap_async.c 
> > b/src/providers/ldap/sdap_async.c
> > index 
> > 97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
> >  100644
> > --- a/src/providers/ldap/sdap_async.c
> > +++ b/src/providers/ldap/sdap_async.c
> > @@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
> >  void *cb_data;
> >  
> >  bool allow_paging;
> > +
> > +unsigned int flags;
> >  };
> >  
> >  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
> > @@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
> > sdap_op *op,
> >   struct sdap_msg *reply,
> >   int error, void *pvt);
> >  
> > +#define WARN_SIZELIMIT 1
> > +
> >  static struct tevent_req *
> > -sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> > -  struct tevent_context *ev,
> > -  struct sdap_options *opts,
> > -  struct sdap_handle *sh,
> > -  const char *search_base,
> > -  int scope,
> > -  const char *filter,
> > -  const char **attrs,
> > -  int attrsonly,
> > -  LDAPControl **serverctrls,
> > -  LDAPControl **clientctrls,
> > -  int sizelimit,
> > -  int timeout,
> > -  bool allow_paging,
> > -  sdap_parse_cb parse_cb,
> > -  void *cb_data)
> > +sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
> > +  struct tevent_context *ev,
> > +  struct sdap_options *opts,
> > +  struct sdap_handle *sh,
> > +  const char *search_base,
> > +  int scope,
> > +  const char *filter,
> > +  const char **attrs,
> > +  int attrsonly,
> > +  LDAPControl **serverctrls,
> > +  LDAPControl **clientctrls,
> > +  int sizelimit,
> > +  int timeout,
> > +  bool allow_paging,
> > +  sdap_parse_cb parse_cb,
> > +  void *cb_data,
> > +  unsigned int flags)
> 
> I don't like us adding another request below sdap_get_generic_ext_send.
> IIRC the idea was that sdap_get_generic_ext_send is really raw and
> unless you need the raw access you should use sdap_get_generic_send
> (without _ext).
> 
> So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
> and let callers deal with the error. The posix check caller would ignore
> it, the others might display an error and carry on.
> 
> The recv in this case needs to first return data and then return the
> error -- maybe this would require us to not use
> TEVENT_REQ_RETURN_ON_ERROR in recv, but unroll its body -- not sure.

I have to admit I do not like returning an error here this way. One
reason is described in the last paragraph, returning an error and data
which might be valid looks odd to me.

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. 

Coming back to returning ERR_SIZELIMIT_EXCEEDED. In general I think it
is a good idea to give the caller as much information as possible. But
here we ignore LDAP_SIZELIMIT_EXCEEDED on purpose to make the life of
the caller easier. Either it is a server side limit and we cannot do
much about it (call which expects lots of results like enumerations
should use paging anyways) or the limit was given by the caller on
purpose (and since we process the data internally 

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

[Jakub Hrozek]

On Wed, Oct 14, 2015 at 12:51:39PM +0200, Sumit Bose wrote:
> On Tue, Oct 06, 2015 at 11:21:54AM +0200, Jakub Hrozek wrote:
> > 
> > I personally don't care about unsigned vs unsigned int, but see my
> > comment about the request inline..
> > 
> > > From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Fri, 25 Sep 2015 07:05:30 -0400
> > > Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
> > > 
> > > Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
> > > be set to suppress warning about 'sizelimit exceeded'.
> > > 
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2804
> > > ---
> > >  src/providers/ldap/sdap_async.c | 78 
> > > +
> > >  1 file changed, 56 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/src/providers/ldap/sdap_async.c 
> > > b/src/providers/ldap/sdap_async.c
> > > index 
> > > 97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
> > >  100644
> > > --- a/src/providers/ldap/sdap_async.c
> > > +++ b/src/providers/ldap/sdap_async.c
> > > @@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
> > >  void *cb_data;
> > >  
> > >  bool allow_paging;
> > > +
> > > +unsigned int flags;
> > >  };
> > >  
> > >  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
> > > @@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
> > > sdap_op *op,
> > >   struct sdap_msg *reply,
> > >   int error, void *pvt);
> > >  
> > > +#define WARN_SIZELIMIT 1
> > > +
> > >  static struct tevent_req *
> > > -sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> > > -  struct tevent_context *ev,
> > > -  struct sdap_options *opts,
> > > -  struct sdap_handle *sh,
> > > -  const char *search_base,
> > > -  int scope,
> > > -  const char *filter,
> > > -  const char **attrs,
> > > -  int attrsonly,
> > > -  LDAPControl **serverctrls,
> > > -  LDAPControl **clientctrls,
> > > -  int sizelimit,
> > > -  int timeout,
> > > -  bool allow_paging,
> > > -  sdap_parse_cb parse_cb,
> > > -  void *cb_data)
> > > +sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
> > > +  struct tevent_context *ev,
> > > +  struct sdap_options *opts,
> > > +  struct sdap_handle *sh,
> > > +  const char *search_base,
> > > +  int scope,
> > > +  const char *filter,
> > > +  const char **attrs,
> > > +  int attrsonly,
> > > +  LDAPControl **serverctrls,
> > > +  LDAPControl **clientctrls,
> > > +  int sizelimit,
> > > +  int timeout,
> > > +  bool allow_paging,
> > > +  sdap_parse_cb parse_cb,
> > > +  void *cb_data,
> > > +  unsigned int flags)
> > 
> > I don't like us adding another request below sdap_get_generic_ext_send.
> > IIRC the idea was that sdap_get_generic_ext_send is really raw and
> > unless you need the raw access you should use sdap_get_generic_send
> > (without _ext).
> > 
> > So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext 
> > request
> > and let callers deal with the error. The posix check caller would ignore
> > it, the others might display an error and carry on.
> > 
> > The recv in this case needs to first return data and then return the
> > error -- maybe this would require us to not use
> > TEVENT_REQ_RETURN_ON_ERROR in recv, but unroll its body -- not sure.
> 
> I have to admit I do not like returning an error here this way. One
> reason is described in the last paragraph, returning an error and data
> which might be valid looks odd to me.
> 
> 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. 
> 
> Coming back to returning ERR_SIZELIMIT_EXCEEDED. In general I think it
> is a good idea to give the caller as much information as possible. But
> here we ignore LDAP_SIZELIMIT_EXCEEDED on purpose 

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

[Pavel Reichl]

On 10/14/2015 01:01 PM, Jakub Hrozek wrote:

On Wed, Oct 14, 2015 at 12:51:39PM +0200, Sumit Bose wrote:

On Tue, Oct 06, 2015 at 11:21:54AM +0200, Jakub Hrozek wrote:


I personally don't care about unsigned vs unsigned int, but see my
comment about the request inline..


 From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
  src/providers/ldap/sdap_async.c | 78 +
  1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 
97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
  void *cb_data;

  bool allow_paging;
+
+unsigned int flags;
  };

  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,
   struct sdap_msg *reply,
   int error, void *pvt);

+#define WARN_SIZELIMIT 1
+
  static struct tevent_req *
-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
-  struct tevent_context *ev,
-  struct sdap_options *opts,
-  struct sdap_handle *sh,
-  const char *search_base,
-  int scope,
-  const char *filter,
-  const char **attrs,
-  int attrsonly,
-  LDAPControl **serverctrls,
-  LDAPControl **clientctrls,
-  int sizelimit,
-  int timeout,
-  bool allow_paging,
-  sdap_parse_cb parse_cb,
-  void *cb_data)
+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
+  struct tevent_context *ev,
+  struct sdap_options *opts,
+  struct sdap_handle *sh,
+  const char *search_base,
+  int scope,
+  const char *filter,
+  const char **attrs,
+  int attrsonly,
+  LDAPControl **serverctrls,
+  LDAPControl **clientctrls,
+  int sizelimit,
+  int timeout,
+  bool allow_paging,
+  sdap_parse_cb parse_cb,
+  void *cb_data,
+  unsigned int flags)


I don't like us adding another request below sdap_get_generic_ext_send.
IIRC the idea was that sdap_get_generic_ext_send is really raw and
unless you need the raw access you should use sdap_get_generic_send
(without _ext).

So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
and let callers deal with the error. The posix check caller would ignore
it, the others might display an error and carry on.

The recv in this case needs to first return data and then return the
error -- maybe this would require us to not use
TEVENT_REQ_RETURN_ON_ERROR in recv, but unroll its body -- not sure.


I have to admit I do not like returning an error here this way. One
reason is described in the last paragraph, returning an error and data
which might be valid looks odd to me.

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.

Coming back to returning ERR_SIZELIMIT_EXCEEDED. In general I think it
is a good idea to give the caller as much information as possible. But
here we ignore LDAP_SIZELIMIT_EXCEEDED on purpose to make the life of
the caller easier. Either it is a server side limit and we cannot do
much about it (call which expects lots of results like enumerations
should use paging anyways) or the limit was given by the caller on
purpose (and since we process the data internally anyway returning
ERR_SIZELIMIT_EXCEEDED would not help the caller much). So we should not
force the caller to ignore the error a second time.

If it becomes really important for a caller to know about

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

[Pavel Reichl]

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!
>From a51a4801fe5a4f78a3e75f27f5055c6cbcda2ce4 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH 1/4] SDAP: optional warning - sizelimit exceeded in POSIX
 check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
 src/providers/ldap/sdap_async.c | 28 +++-
 1 file changed, 19 insertions(+), 9 deletions(-)

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;
 };
 
 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_SILENT0x01
+
 static struct tevent_req *
 sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   struct tevent_context *ev,
@@ -1188,7 +1192,8 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   int timeout,
   bool allow_paging,
   sdap_parse_cb parse_cb,
-  void *cb_data)
+  void *cb_data,
+  unsigned int flags)
 {
 errno_t ret;
 struct sdap_get_generic_ext_state *state;
@@ -1215,6 +1220,7 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
 state->parse_cb = parse_cb;
 state->cb_data = cb_data;
 state->clientctrls = clientctrls;
+state->sizelimit_silent = flags & SDAP_SRCH_FLG_SIZELIMIT_SILENT;
 
 if (state->sh == NULL || state->sh->ldap == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE,
@@ -1500,8 +1506,11 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
 
 if (result == LDAP_SIZELIMIT_EXCEEDED) {
 /* Try to return what we've got */
-DEBUG(SSSDBG_MINOR_FAILURE,
-  "LDAP sizelimit was exceeded, returning incomplete data\n");
+if (state->sizelimit_silent == false) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  "LDAP sizelimit was exceeded, "
+  "returning incomplete data\n");
+}
 } else if (result == LDAP_INAPPROPRIATE_MATCHING) {
 /* This error should only occur when we're testing for
  * specialized functionality like the ldap matching rule
@@ -1708,7 +1717,8 @@ struct tevent_req *sdap_get_and_parse_generic_send(TALLOC_CTX *memctx,
 subreq = sdap_get_generic_ext_send(state, ev, opts, sh, search_base,
scope, filter, attrs, false, NULL,
NULL, sizelimit, timeout, allow_paging,
-   sdap_get_and_parse_generic_parse_entry, state);
+   sdap_get_and_parse_generic_parse_entry,
+   state, 0);
 if (!subreq) {
 talloc_zfree(req);
   

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

[Pavel Reichl]

On 10/12/2015 07:50 AM, Lukas Slebodnik wrote:

On (09/10/15 20:02), Michal Židek wrote:

On 10/09/2015 02:05 PM, Lukas Slebodnik wrote:

On (09/10/15 13:56), Jakub Hrozek wrote:

On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:



On 10/06/2015 11:21 AM, Jakub Hrozek wrote:


I personally don't care about unsigned vs unsigned int, but see my
comment about the request inline..


 From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
  src/providers/ldap/sdap_async.c | 78 +
  1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 
97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
  void *cb_data;

  bool allow_paging;
+
+unsigned int flags;
  };

  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,
   struct sdap_msg *reply,
   int error, void *pvt);

+#define WARN_SIZELIMIT 1
+
  static struct tevent_req *
-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
-  struct tevent_context *ev,
-  struct sdap_options *opts,
-  struct sdap_handle *sh,
-  const char *search_base,
-  int scope,
-  const char *filter,
-  const char **attrs,
-  int attrsonly,
-  LDAPControl **serverctrls,
-  LDAPControl **clientctrls,
-  int sizelimit,
-  int timeout,
-  bool allow_paging,
-  sdap_parse_cb parse_cb,
-  void *cb_data)
+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
+  struct tevent_context *ev,
+  struct sdap_options *opts,
+  struct sdap_handle *sh,
+  const char *search_base,
+  int scope,
+  const char *filter,
+  const char **attrs,
+  int attrsonly,
+  LDAPControl **serverctrls,
+  LDAPControl **clientctrls,
+  int sizelimit,
+  int timeout,
+  bool allow_paging,
+  sdap_parse_cb parse_cb,
+  void *cb_data,
+  unsigned int flags)


I don't like us adding another request below sdap_get_generic_ext_send.


OK, I understand that.


IIRC the idea was that sdap_get_generic_ext_send is really raw and
unless you need the raw access you should use sdap_get_generic_send
(without _ext).

So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
and let callers deal with the error. The posix check caller would ignore
it, the others might display an error and carry on.

This sound to me like code duplication. I would have to add to several calls 
same check and print same message.


This is totally untested version of what I meant:
https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc


It looks more elegant then adding flags to function.
So +1

But there is still a change that it does not work because patches are totally
untested :-)

LS


I agree with Pavel that ignoring the debug message is non standard
thing that we want to do only during the posix check and it does not
add much readbility/ease of use when the caller needs to stalk for
the special error code (only in one case it will be ignored).
I also agree with Jakub that we do not want to expand already big
number of arguments.

Please see the attached patch. It is less invasive and only changes
parameters for the posix check (for now the one and only special
case when we want to ignore the debug message).

Michal

PS: The patch is also totally untested and serves for illustration,
but it is simple enough to accidentally work :)



From e09e4f368cf63f72ac218b9104c9486f8ad339f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= 
Date: Fri, 9 Oct 2015 19:24:40 +0200

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

[Pavel Reichl]

On 10/09/2015 08:02 PM, Michal Židek wrote:

On 10/09/2015 02:05 PM, Lukas Slebodnik wrote:

On (09/10/15 13:56), Jakub Hrozek wrote:

On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:



On 10/06/2015 11:21 AM, Jakub Hrozek wrote:


I personally don't care about unsigned vs unsigned int, but see my
comment about the request inline..


 From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
  src/providers/ldap/sdap_async.c | 78 +
  1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 
97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
  void *cb_data;

  bool allow_paging;
+
+unsigned int flags;
  };

  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,
   struct sdap_msg *reply,
   int error, void *pvt);

+#define WARN_SIZELIMIT 1
+
  static struct tevent_req *
-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
-  struct tevent_context *ev,
-  struct sdap_options *opts,
-  struct sdap_handle *sh,
-  const char *search_base,
-  int scope,
-  const char *filter,
-  const char **attrs,
-  int attrsonly,
-  LDAPControl **serverctrls,
-  LDAPControl **clientctrls,
-  int sizelimit,
-  int timeout,
-  bool allow_paging,
-  sdap_parse_cb parse_cb,
-  void *cb_data)
+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
+  struct tevent_context *ev,
+  struct sdap_options *opts,
+  struct sdap_handle *sh,
+  const char *search_base,
+  int scope,
+  const char *filter,
+  const char **attrs,
+  int attrsonly,
+  LDAPControl **serverctrls,
+  LDAPControl **clientctrls,
+  int sizelimit,
+  int timeout,
+  bool allow_paging,
+  sdap_parse_cb parse_cb,
+  void *cb_data,
+  unsigned int flags)


I don't like us adding another request below sdap_get_generic_ext_send.


OK, I understand that.


IIRC the idea was that sdap_get_generic_ext_send is really raw and
unless you need the raw access you should use sdap_get_generic_send
(without _ext).

So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
and let callers deal with the error. The posix check caller would ignore
it, the others might display an error and carry on.

This sound to me like code duplication. I would have to add to several calls 
same check and print same message.


This is totally untested version of what I meant:
https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc


It looks more elegant then adding flags to function.
So +1

But there is still a change that it does not work because patches are totally
untested :-)

LS


I agree with Pavel that ignoring the debug message is non standard
thing that we want to do only during the posix check and it does not
add much readbility/ease of use when the caller needs to stalk for
the special error code (only in one case it will be ignored).
I also agree with Jakub that we do not want to expand already big
number of arguments.

Please see the attached patch. It is less invasive and only changes
parameters for the posix check (for now the one and only special
case when we want to ignore the debug message).


Thanks for the idea. It's definitely clever but it's IMO a hack and I think we 
can handle this without one.



Michal

PS: The patch is also totally untested and serves for illustration,
but it is simple enough to accidentally work :)


___
sssd-devel mailing list

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

[Petr Cech]

Hi everyone,

we just discussed 'function wrapper' topic offline.

I agree that it is not ideal to add new parameter to the function. And I 
agree that in languages like C, we have return value model.


On the other hand, we have clean code on our minds. So I think that 
wrappers like:


#  int func(a, b, c, d);
#  int func_with_warns(a,b,c,d);

are better if we use func() very often. Why? The reason is that we look 
at func() as to something which do one thing or one thing with printing 
warnings. So we can quickly check if every occurrences are right or not. 
It could be confusing if we needed to check something like:


#  int ret;
#  ret = func(a,b,c,d);
#  if (ret != EOK) {
#  }
#  ...
#  if (ret == WARNS) {
#LOG(...);
#  }

Regards

Petr

PS: I didn't read the thread, so it is only my 2 cents.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Jakub Hrozek]

On Mon, Oct 12, 2015 at 07:50:39AM +0200, Lukas Slebodnik wrote:
> The returned error code is equivalent to exception in high level programming
> languages. If you don't know how to handle error in such languages you just
> throw/raise an exception and caller function should handle such error.

FWIW, this is more or less what I had in mind, but I'm equally OK with
any solution that doesn't increase the signature of the function with
single-purpose booleans.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Jakub Hrozek]

On Fri, Oct 09, 2015 at 08:02:08PM +0200, Michal Židek wrote:
> On 10/09/2015 02:05 PM, Lukas Slebodnik wrote:
> >On (09/10/15 13:56), Jakub Hrozek wrote:
> >>On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:
> >>>
> >>>
> >>>On 10/06/2015 11:21 AM, Jakub Hrozek wrote:
> 
> I personally don't care about unsigned vs unsigned int, but see my
> comment about the request inline..
> 
> > From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl 
> >Date: Fri, 25 Sep 2015 07:05:30 -0400
> >Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
> >
> >Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
> >be set to suppress warning about 'sizelimit exceeded'.
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/2804
> >---
> >  src/providers/ldap/sdap_async.c | 78 
> > +
> >  1 file changed, 56 insertions(+), 22 deletions(-)
> >
> >diff --git a/src/providers/ldap/sdap_async.c 
> >b/src/providers/ldap/sdap_async.c
> >index 
> >97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
> > 100644
> >--- a/src/providers/ldap/sdap_async.c
> >+++ b/src/providers/ldap/sdap_async.c
> >@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
> >  void *cb_data;
> >
> >  bool allow_paging;
> >+
> >+unsigned int flags;
> >  };
> >
> >  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
> >@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
> >sdap_op *op,
> >   struct sdap_msg *reply,
> >   int error, void *pvt);
> >
> >+#define WARN_SIZELIMIT 1
> >+
> >  static struct tevent_req *
> >-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> >-  struct tevent_context *ev,
> >-  struct sdap_options *opts,
> >-  struct sdap_handle *sh,
> >-  const char *search_base,
> >-  int scope,
> >-  const char *filter,
> >-  const char **attrs,
> >-  int attrsonly,
> >-  LDAPControl **serverctrls,
> >-  LDAPControl **clientctrls,
> >-  int sizelimit,
> >-  int timeout,
> >-  bool allow_paging,
> >-  sdap_parse_cb parse_cb,
> >-  void *cb_data)
> >+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
> >+  struct tevent_context *ev,
> >+  struct sdap_options *opts,
> >+  struct sdap_handle *sh,
> >+  const char *search_base,
> >+  int scope,
> >+  const char *filter,
> >+  const char **attrs,
> >+  int attrsonly,
> >+  LDAPControl **serverctrls,
> >+  LDAPControl **clientctrls,
> >+  int sizelimit,
> >+  int timeout,
> >+  bool allow_paging,
> >+  sdap_parse_cb parse_cb,
> >+  void *cb_data,
> >+  unsigned int flags)
> 
> I don't like us adding another request below sdap_get_generic_ext_send.
> >>>
> >>>OK, I understand that.
> >>>
> IIRC the idea was that sdap_get_generic_ext_send is really raw and
> unless you need the raw access you should use sdap_get_generic_send
> (without _ext).
> 
> So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext 
> request
> and let callers deal with the error. The posix check caller would ignore
> it, the others might display an error and carry on.
> >>>This sound to me like code duplication. I would have to add to several 
> >>>calls same check and print same message.
> >>
> >>This is totally untested version of what I meant:
> >>https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc
> >>
> >It looks more elegant then adding flags to function.
> >So +1
> >
> >But there is still a change that it does not work because patches are totally
> >untested :-)
> >
> >LS
> 
> I agree with Pavel that ignoring the debug message is non standard
> thing that we want to do only during the posix check and it does not
> add much readbility/ease of use when the caller 

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

[Lukas Slebodnik]

On (09/10/15 20:02), Michal Židek wrote:
>On 10/09/2015 02:05 PM, Lukas Slebodnik wrote:
>>On (09/10/15 13:56), Jakub Hrozek wrote:
>>>On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:


On 10/06/2015 11:21 AM, Jakub Hrozek wrote:
>
>I personally don't care about unsigned vs unsigned int, but see my
>comment about the request inline..
>
>> From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
>>From: Pavel Reichl 
>>Date: Fri, 25 Sep 2015 07:05:30 -0400
>>Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
>>
>>Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
>>be set to suppress warning about 'sizelimit exceeded'.
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2804
>>---
>>  src/providers/ldap/sdap_async.c | 78 
>> +
>>  1 file changed, 56 insertions(+), 22 deletions(-)
>>
>>diff --git a/src/providers/ldap/sdap_async.c 
>>b/src/providers/ldap/sdap_async.c
>>index 
>>97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
>> 100644
>>--- a/src/providers/ldap/sdap_async.c
>>+++ b/src/providers/ldap/sdap_async.c
>>@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
>>  void *cb_data;
>>
>>  bool allow_paging;
>>+
>>+unsigned int flags;
>>  };
>>
>>  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
>>@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
>>sdap_op *op,
>>   struct sdap_msg *reply,
>>   int error, void *pvt);
>>
>>+#define WARN_SIZELIMIT 1
>>+
>>  static struct tevent_req *
>>-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>>-  struct tevent_context *ev,
>>-  struct sdap_options *opts,
>>-  struct sdap_handle *sh,
>>-  const char *search_base,
>>-  int scope,
>>-  const char *filter,
>>-  const char **attrs,
>>-  int attrsonly,
>>-  LDAPControl **serverctrls,
>>-  LDAPControl **clientctrls,
>>-  int sizelimit,
>>-  int timeout,
>>-  bool allow_paging,
>>-  sdap_parse_cb parse_cb,
>>-  void *cb_data)
>>+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
>>+  struct tevent_context *ev,
>>+  struct sdap_options *opts,
>>+  struct sdap_handle *sh,
>>+  const char *search_base,
>>+  int scope,
>>+  const char *filter,
>>+  const char **attrs,
>>+  int attrsonly,
>>+  LDAPControl **serverctrls,
>>+  LDAPControl **clientctrls,
>>+  int sizelimit,
>>+  int timeout,
>>+  bool allow_paging,
>>+  sdap_parse_cb parse_cb,
>>+  void *cb_data,
>>+  unsigned int flags)
>
>I don't like us adding another request below sdap_get_generic_ext_send.

OK, I understand that.

>IIRC the idea was that sdap_get_generic_ext_send is really raw and
>unless you need the raw access you should use sdap_get_generic_send
>(without _ext).
>
>So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext 
>request
>and let callers deal with the error. The posix check caller would ignore
>it, the others might display an error and carry on.
This sound to me like code duplication. I would have to add to several 
calls same check and print same message.
>>>
>>>This is totally untested version of what I meant:
>>>https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc
>>>
>>It looks more elegant then adding flags to function.
>>So +1
>>
>>But there is still a change that it does not work because patches are totally
>>untested :-)
>>
>>LS
>
>I agree with Pavel that ignoring the debug message is non standard
>thing that we want to do only during the posix check and it does not
>add much readbility/ease of use when the caller needs to stalk for
>the special error code (only in one case it will be ignored).
>I also agree with Jakub that we do not want to expand 

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

[Jakub Hrozek]

On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:
> 
> 
> On 10/06/2015 11:21 AM, Jakub Hrozek wrote:
> >
> >I personally don't care about unsigned vs unsigned int, but see my
> >comment about the request inline..
> >
> >> From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
> >>From: Pavel Reichl 
> >>Date: Fri, 25 Sep 2015 07:05:30 -0400
> >>Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
> >>
> >>Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
> >>be set to suppress warning about 'sizelimit exceeded'.
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2804
> >>---
> >>  src/providers/ldap/sdap_async.c | 78 
> >> +
> >>  1 file changed, 56 insertions(+), 22 deletions(-)
> >>
> >>diff --git a/src/providers/ldap/sdap_async.c 
> >>b/src/providers/ldap/sdap_async.c
> >>index 
> >>97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
> >> 100644
> >>--- a/src/providers/ldap/sdap_async.c
> >>+++ b/src/providers/ldap/sdap_async.c
> >>@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
> >>  void *cb_data;
> >>
> >>  bool allow_paging;
> >>+
> >>+unsigned int flags;
> >>  };
> >>
> >>  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
> >>@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
> >>sdap_op *op,
> >>   struct sdap_msg *reply,
> >>   int error, void *pvt);
> >>
> >>+#define WARN_SIZELIMIT 1
> >>+
> >>  static struct tevent_req *
> >>-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> >>-  struct tevent_context *ev,
> >>-  struct sdap_options *opts,
> >>-  struct sdap_handle *sh,
> >>-  const char *search_base,
> >>-  int scope,
> >>-  const char *filter,
> >>-  const char **attrs,
> >>-  int attrsonly,
> >>-  LDAPControl **serverctrls,
> >>-  LDAPControl **clientctrls,
> >>-  int sizelimit,
> >>-  int timeout,
> >>-  bool allow_paging,
> >>-  sdap_parse_cb parse_cb,
> >>-  void *cb_data)
> >>+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
> >>+  struct tevent_context *ev,
> >>+  struct sdap_options *opts,
> >>+  struct sdap_handle *sh,
> >>+  const char *search_base,
> >>+  int scope,
> >>+  const char *filter,
> >>+  const char **attrs,
> >>+  int attrsonly,
> >>+  LDAPControl **serverctrls,
> >>+  LDAPControl **clientctrls,
> >>+  int sizelimit,
> >>+  int timeout,
> >>+  bool allow_paging,
> >>+  sdap_parse_cb parse_cb,
> >>+  void *cb_data,
> >>+  unsigned int flags)
> >
> >I don't like us adding another request below sdap_get_generic_ext_send.
> 
> OK, I understand that.
> 
> >IIRC the idea was that sdap_get_generic_ext_send is really raw and
> >unless you need the raw access you should use sdap_get_generic_send
> >(without _ext).
> >
> >So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
> >and let callers deal with the error. The posix check caller would ignore
> >it, the others might display an error and carry on.
> This sound to me like code duplication. I would have to add to several calls 
> same check and print same message.

This is totally untested version of what I meant:
https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc

> 
> I believe it's better to instruct directly sdap_get_generic_ext_send() not
> to print the warning. I understand that adding parameters to functions is
> not desired, but I think that in this case it's justified. I could even make
> parameter allow_paging to be part of the flag and thus don't increase the
> number of parameters at all.

If you still prefer your version, then I guess it would be better to
converge on one parameter to control the behaviour (but I wouldn't
change allow_paging in sdap_get_and_parse_generic_send)

> 
> >
> >The recv in this case needs to first return data and then return the
> >error -- maybe this would require us to not use
> >TEVENT_REQ_RETURN_ON_ERROR in recv, but unroll its body -- not sure.
> 
> Yes, I believe your proposed solution would work, but I'm worried that it 
> might be more 

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

[Jakub Hrozek]

On Fri, Oct 09, 2015 at 01:56:22PM +0200, Jakub Hrozek wrote:
> On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:
> > 
> > 
> > On 10/06/2015 11:21 AM, Jakub Hrozek wrote:
> > >
> > >I personally don't care about unsigned vs unsigned int, but see my
> > >comment about the request inline..
> > >
> > >> From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
> > >>From: Pavel Reichl 
> > >>Date: Fri, 25 Sep 2015 07:05:30 -0400
> > >>Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
> > >>
> > >>Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
> > >>be set to suppress warning about 'sizelimit exceeded'.
> > >>
> > >>Resolves:
> > >>https://fedorahosted.org/sssd/ticket/2804
> > >>---
> > >>  src/providers/ldap/sdap_async.c | 78 
> > >> +
> > >>  1 file changed, 56 insertions(+), 22 deletions(-)
> > >>
> > >>diff --git a/src/providers/ldap/sdap_async.c 
> > >>b/src/providers/ldap/sdap_async.c
> > >>index 
> > >>97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
> > >> 100644
> > >>--- a/src/providers/ldap/sdap_async.c
> > >>+++ b/src/providers/ldap/sdap_async.c
> > >>@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
> > >>  void *cb_data;
> > >>
> > >>  bool allow_paging;
> > >>+
> > >>+unsigned int flags;
> > >>  };
> > >>
> > >>  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
> > >>@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
> > >>sdap_op *op,
> > >>   struct sdap_msg *reply,
> > >>   int error, void *pvt);
> > >>
> > >>+#define WARN_SIZELIMIT 1
> > >>+
> > >>  static struct tevent_req *
> > >>-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> > >>-  struct tevent_context *ev,
> > >>-  struct sdap_options *opts,
> > >>-  struct sdap_handle *sh,
> > >>-  const char *search_base,
> > >>-  int scope,
> > >>-  const char *filter,
> > >>-  const char **attrs,
> > >>-  int attrsonly,
> > >>-  LDAPControl **serverctrls,
> > >>-  LDAPControl **clientctrls,
> > >>-  int sizelimit,
> > >>-  int timeout,
> > >>-  bool allow_paging,
> > >>-  sdap_parse_cb parse_cb,
> > >>-  void *cb_data)
> > >>+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
> > >>+  struct tevent_context *ev,
> > >>+  struct sdap_options *opts,
> > >>+  struct sdap_handle *sh,
> > >>+  const char *search_base,
> > >>+  int scope,
> > >>+  const char *filter,
> > >>+  const char **attrs,
> > >>+  int attrsonly,
> > >>+  LDAPControl **serverctrls,
> > >>+  LDAPControl **clientctrls,
> > >>+  int sizelimit,
> > >>+  int timeout,
> > >>+  bool allow_paging,
> > >>+  sdap_parse_cb parse_cb,
> > >>+  void *cb_data,
> > >>+  unsigned int flags)
> > >
> > >I don't like us adding another request below sdap_get_generic_ext_send.
> > 
> > OK, I understand that.
> > 
> > >IIRC the idea was that sdap_get_generic_ext_send is really raw and
> > >unless you need the raw access you should use sdap_get_generic_send
> > >(without _ext).
> > >
> > >So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext 
> > >request
> > >and let callers deal with the error. The posix check caller would ignore
> > >it, the others might display an error and carry on.
> > This sound to me like code duplication. I would have to add to several 
> > calls same check and print same message.
> 
> This is totally untested version of what I meant:
> https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc
> 
> > 
> > I believe it's better to instruct directly sdap_get_generic_ext_send() not
> > to print the warning. I understand that adding parameters to functions is
> > not desired, but I think that in this case it's justified. I could even make
> > parameter allow_paging to be part of the flag and thus don't increase the
> > number of parameters at all.

It would also be better if the flags definition was hexadecimal and
namespaced:
#define SDAP_SRCH_FLG_PAGING  0x01
#define SDAP_SRCH_FLG_SIZELIMIT_SILENT0x02
#define SDAP_SRCH_FLG_FOOBAR 

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

[Lukas Slebodnik]

On (09/10/15 13:56), Jakub Hrozek wrote:
>On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:
>> 
>> 
>> On 10/06/2015 11:21 AM, Jakub Hrozek wrote:
>> >
>> >I personally don't care about unsigned vs unsigned int, but see my
>> >comment about the request inline..
>> >
>> >> From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
>> >>From: Pavel Reichl 
>> >>Date: Fri, 25 Sep 2015 07:05:30 -0400
>> >>Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
>> >>
>> >>Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
>> >>be set to suppress warning about 'sizelimit exceeded'.
>> >>
>> >>Resolves:
>> >>https://fedorahosted.org/sssd/ticket/2804
>> >>---
>> >>  src/providers/ldap/sdap_async.c | 78 
>> >> +
>> >>  1 file changed, 56 insertions(+), 22 deletions(-)
>> >>
>> >>diff --git a/src/providers/ldap/sdap_async.c 
>> >>b/src/providers/ldap/sdap_async.c
>> >>index 
>> >>97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
>> >> 100644
>> >>--- a/src/providers/ldap/sdap_async.c
>> >>+++ b/src/providers/ldap/sdap_async.c
>> >>@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
>> >>  void *cb_data;
>> >>
>> >>  bool allow_paging;
>> >>+
>> >>+unsigned int flags;
>> >>  };
>> >>
>> >>  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
>> >>@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
>> >>sdap_op *op,
>> >>   struct sdap_msg *reply,
>> >>   int error, void *pvt);
>> >>
>> >>+#define WARN_SIZELIMIT 1
>> >>+
>> >>  static struct tevent_req *
>> >>-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>> >>-  struct tevent_context *ev,
>> >>-  struct sdap_options *opts,
>> >>-  struct sdap_handle *sh,
>> >>-  const char *search_base,
>> >>-  int scope,
>> >>-  const char *filter,
>> >>-  const char **attrs,
>> >>-  int attrsonly,
>> >>-  LDAPControl **serverctrls,
>> >>-  LDAPControl **clientctrls,
>> >>-  int sizelimit,
>> >>-  int timeout,
>> >>-  bool allow_paging,
>> >>-  sdap_parse_cb parse_cb,
>> >>-  void *cb_data)
>> >>+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
>> >>+  struct tevent_context *ev,
>> >>+  struct sdap_options *opts,
>> >>+  struct sdap_handle *sh,
>> >>+  const char *search_base,
>> >>+  int scope,
>> >>+  const char *filter,
>> >>+  const char **attrs,
>> >>+  int attrsonly,
>> >>+  LDAPControl **serverctrls,
>> >>+  LDAPControl **clientctrls,
>> >>+  int sizelimit,
>> >>+  int timeout,
>> >>+  bool allow_paging,
>> >>+  sdap_parse_cb parse_cb,
>> >>+  void *cb_data,
>> >>+  unsigned int flags)
>> >
>> >I don't like us adding another request below sdap_get_generic_ext_send.
>> 
>> OK, I understand that.
>> 
>> >IIRC the idea was that sdap_get_generic_ext_send is really raw and
>> >unless you need the raw access you should use sdap_get_generic_send
>> >(without _ext).
>> >
>> >So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext 
>> >request
>> >and let callers deal with the error. The posix check caller would ignore
>> >it, the others might display an error and carry on.
>> This sound to me like code duplication. I would have to add to several calls 
>> same check and print same message.
>
>This is totally untested version of what I meant:
>https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc
>
It looks more elegant then adding flags to function.
So +1

But there is still a change that it does not work because patches are totally
untested :-)

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Michal Židek]

On 10/09/2015 02:05 PM, Lukas Slebodnik wrote:

On (09/10/15 13:56), Jakub Hrozek wrote:

On Tue, Oct 06, 2015 at 01:25:33PM +0200, Pavel Reichl wrote:



On 10/06/2015 11:21 AM, Jakub Hrozek wrote:


I personally don't care about unsigned vs unsigned int, but see my
comment about the request inline..


 From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
  src/providers/ldap/sdap_async.c | 78 +
  1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 
97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
  void *cb_data;

  bool allow_paging;
+
+unsigned int flags;
  };

  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,
   struct sdap_msg *reply,
   int error, void *pvt);

+#define WARN_SIZELIMIT 1
+
  static struct tevent_req *
-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
-  struct tevent_context *ev,
-  struct sdap_options *opts,
-  struct sdap_handle *sh,
-  const char *search_base,
-  int scope,
-  const char *filter,
-  const char **attrs,
-  int attrsonly,
-  LDAPControl **serverctrls,
-  LDAPControl **clientctrls,
-  int sizelimit,
-  int timeout,
-  bool allow_paging,
-  sdap_parse_cb parse_cb,
-  void *cb_data)
+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
+  struct tevent_context *ev,
+  struct sdap_options *opts,
+  struct sdap_handle *sh,
+  const char *search_base,
+  int scope,
+  const char *filter,
+  const char **attrs,
+  int attrsonly,
+  LDAPControl **serverctrls,
+  LDAPControl **clientctrls,
+  int sizelimit,
+  int timeout,
+  bool allow_paging,
+  sdap_parse_cb parse_cb,
+  void *cb_data,
+  unsigned int flags)


I don't like us adding another request below sdap_get_generic_ext_send.


OK, I understand that.


IIRC the idea was that sdap_get_generic_ext_send is really raw and
unless you need the raw access you should use sdap_get_generic_send
(without _ext).

So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
and let callers deal with the error. The posix check caller would ignore
it, the others might display an error and carry on.

This sound to me like code duplication. I would have to add to several calls 
same check and print same message.


This is totally untested version of what I meant:
https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=misc


It looks more elegant then adding flags to function.
So +1

But there is still a change that it does not work because patches are totally
untested :-)

LS


I agree with Pavel that ignoring the debug message is non standard
thing that we want to do only during the posix check and it does not
add much readbility/ease of use when the caller needs to stalk for
the special error code (only in one case it will be ignored).
I also agree with Jakub that we do not want to expand already big
number of arguments.

Please see the attached patch. It is less invasive and only changes
parameters for the posix check (for now the one and only special
case when we want to ignore the debug message).

Michal

PS: The patch is also totally untested and serves for illustration,
but it is simple enough to accidentally work :)
>From e09e4f368cf63f72ac218b9104c9486f8ad339f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= 
Date: Fri, 9 Oct 2015 19:24:40 +0200
Subject: [PATCH] sdap: Ignore exceeding sizelimit during posix check

Esceeding sizelimit during 

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

[Jakub Hrozek]

I personally don't care about unsigned vs unsigned int, but see my
comment about the request inline..

> From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Fri, 25 Sep 2015 07:05:30 -0400
> Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
> 
> Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
> be set to suppress warning about 'sizelimit exceeded'.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2804
> ---
>  src/providers/ldap/sdap_async.c | 78 
> +
>  1 file changed, 56 insertions(+), 22 deletions(-)
> 
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index 
> 97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
>  100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
>  void *cb_data;
>  
>  bool allow_paging;
> +
> +unsigned int flags;
>  };
>  
>  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
> @@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct 
> sdap_op *op,
>   struct sdap_msg *reply,
>   int error, void *pvt);
>  
> +#define WARN_SIZELIMIT 1
> +
>  static struct tevent_req *
> -sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> -  struct tevent_context *ev,
> -  struct sdap_options *opts,
> -  struct sdap_handle *sh,
> -  const char *search_base,
> -  int scope,
> -  const char *filter,
> -  const char **attrs,
> -  int attrsonly,
> -  LDAPControl **serverctrls,
> -  LDAPControl **clientctrls,
> -  int sizelimit,
> -  int timeout,
> -  bool allow_paging,
> -  sdap_parse_cb parse_cb,
> -  void *cb_data)
> +sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
> +  struct tevent_context *ev,
> +  struct sdap_options *opts,
> +  struct sdap_handle *sh,
> +  const char *search_base,
> +  int scope,
> +  const char *filter,
> +  const char **attrs,
> +  int attrsonly,
> +  LDAPControl **serverctrls,
> +  LDAPControl **clientctrls,
> +  int sizelimit,
> +  int timeout,
> +  bool allow_paging,
> +  sdap_parse_cb parse_cb,
> +  void *cb_data,
> +  unsigned int flags)

I don't like us adding another request below sdap_get_generic_ext_send.
IIRC the idea was that sdap_get_generic_ext_send is really raw and
unless you need the raw access you should use sdap_get_generic_send
(without _ext).

So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
and let callers deal with the error. The posix check caller would ignore
it, the others might display an error and carry on.

The recv in this case needs to first return data and then return the
error -- maybe this would require us to not use
TEVENT_REQ_RETURN_ON_ERROR in recv, but unroll its body -- not sure.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Pavel Reichl]

On 10/06/2015 11:21 AM, Jakub Hrozek wrote:


I personally don't care about unsigned vs unsigned int, but see my
comment about the request inline..


 From 2281410185205ab3fc483f4c45b1b1378b62c331 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
  src/providers/ldap/sdap_async.c | 78 +
  1 file changed, 56 insertions(+), 22 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 
97c9ea5df61a6516ca74bb73edc9a116b1266c71..0044ea9c319af08a87fa59e89559851ad6e8abc5
 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
  void *cb_data;

  bool allow_paging;
+
+unsigned int flags;
  };

  static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,23 +1174,26 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,
   struct sdap_msg *reply,
   int error, void *pvt);

+#define WARN_SIZELIMIT 1
+
  static struct tevent_req *
-sdap_get_generic_ext_send(TALLOC_CTX *memctx,
-  struct tevent_context *ev,
-  struct sdap_options *opts,
-  struct sdap_handle *sh,
-  const char *search_base,
-  int scope,
-  const char *filter,
-  const char **attrs,
-  int attrsonly,
-  LDAPControl **serverctrls,
-  LDAPControl **clientctrls,
-  int sizelimit,
-  int timeout,
-  bool allow_paging,
-  sdap_parse_cb parse_cb,
-  void *cb_data)
+sdap_get_generic_ext_send_ext(TALLOC_CTX *memctx,
+  struct tevent_context *ev,
+  struct sdap_options *opts,
+  struct sdap_handle *sh,
+  const char *search_base,
+  int scope,
+  const char *filter,
+  const char **attrs,
+  int attrsonly,
+  LDAPControl **serverctrls,
+  LDAPControl **clientctrls,
+  int sizelimit,
+  int timeout,
+  bool allow_paging,
+  sdap_parse_cb parse_cb,
+  void *cb_data,
+  unsigned int flags)


I don't like us adding another request below sdap_get_generic_ext_send.


OK, I understand that.


IIRC the idea was that sdap_get_generic_ext_send is really raw and
unless you need the raw access you should use sdap_get_generic_send
(without _ext).

So here I would prefer to return ERR_SIZELIMIT_EXCEEDED from the _ext request
and let callers deal with the error. The posix check caller would ignore
it, the others might display an error and carry on.

This sound to me like code duplication. I would have to add to several calls 
same check and print same message.

I believe it's better to instruct directly sdap_get_generic_ext_send() not to print the warning. I understand that adding parameters to functions is not desired, but I think that in this case it's justified. I could even make parameter allow_paging 
to be part of the flag and thus don't increase the number of parameters at all.




The recv in this case needs to first return data and then return the
error -- maybe this would require us to not use
TEVENT_REQ_RETURN_ON_ERROR in recv, but unroll its body -- not sure.


Yes, I believe your proposed solution would work, but I'm worried that it might 
be more robust change and it could be more error prone.

Please reconsider attached patch.

Thanks!


>From ef34acd1bfaade490a54d563a9e46bce5cde1479 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send_ext() which can
be set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
 src/providers/ldap/sdap_async.c | 28 +++-
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 

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

[Pavel Reichl]

On 09/29/2015 08:46 AM, Lukas Slebodnik wrote:

On (25/09/15 14:31), Pavel Reichl wrote:



On 09/25/2015 01:59 PM, Lukas Slebodnik wrote:

On (25/09/15 13:30), Pavel Reichl wrote:

Hello,

please see simple patch attached.

Thanks!


>From 5717c6effcb0ac0cd16b4863adba088c9b1f0b90 Mon Sep 17 00:00:00 2001

From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send() which can be
set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
src/providers/ldap/sdap_async.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 
97c9ea5df61a6516ca74bb73edc9a116b1266c71..1626d0dada1800aaf69681de13632b007866e779
 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
 void *cb_data;

 bool allow_paging;
+
+unsigned int flags;

^^^
 The int is redundant here
 "unsigned int" and "unsigned" is the same.


Indeed it's the same. But we use this form quite common. Do you want me to file 
a ticket and assign it to you?


No,
we just needn't untroduce new versions.


Do we have some kind of agreement which version is preferred? Apparently 
unsigned int is used more often...


The sort variant is already used in sssd
e.g.
src/tests/cmocka/test_nss_srv.c:static void 
mock_fill_group_with_members(unsigned members)
src/tests/cmocka/test_nss_srv.c:unsigned i;
src/tests/cmocka/test_nss_srv.c:unsigned i;
src/tests/cmocka/test_nss_srv.c:unsigned i;
src/tests/cmocka/test_sysdb_views.c: 
unsigned override_gid)
src/tests/cmocka/test_sysdb_views.c:   unsigned 
expected_override_gid)
src/tests/cmocka/test_sysdb_views.c:unsigned gid;


git grep 'unsigned int' |  wc -l
206





};

static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,6 +1174,14 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,
  struct sdap_msg *reply,
  int error, void *pvt);

+#define NO_FLAGS 0
+#define DONT_WARN_SIZELIMIT_EXCEEDED 1
+
+static bool is_flag_set(unsigned int flags, unsigned int flag)
+{
+return (flags & flag) != 0;
+}
+
static struct tevent_req *
sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   struct tevent_context *ev,
@@ -1188,7 +1198,8 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   int timeout,
   bool allow_paging,
   sdap_parse_cb parse_cb,
-  void *cb_data)
+  void *cb_data,
+  unsigned int flags)
{
 errno_t ret;
 struct sdap_get_generic_ext_state *state;
@@ -1215,6 +1226,7 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
 state->parse_cb = parse_cb;
 state->cb_data = cb_data;
 state->clientctrls = clientctrls;
+state->flags = flags;

 if (state->sh == NULL || state->sh->ldap == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE,
@@ -1500,8 +1512,11 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,

 if (result == LDAP_SIZELIMIT_EXCEEDED) {
 /* Try to return what we've got */
-DEBUG(SSSDBG_MINOR_FAILURE,
-  "LDAP sizelimit was exceeded, returning incomplete data\n");
+if (!is_flag_set(state->flags, DONT_WARN_SIZELIMIT_EXCEEDED)) {

The function is used just once and it does not significantly
improve readability of code; But it might be caused by too

As negation is needed I believe it improves readability enough to advocate it's 
existence.


long name of flag and many brackets + negation.
   if (state->flags & WARN_SIZELIMIT) might be easier to read.


I don't like WARN_SIZELIMIT because setting any flag should not be required by 
default and this flag would imply otherwise.


You still need to you "flag" NO_FLAGS. So the argument with default
should not be excuse for not improving readability of code.


I'm not looking for excuses not to improve code readability on contrary I'm 
doing my best to improve it.

In attached patch I removed the is_flag_set() function and somehow solved 
problem with default flags.




BTW It might be caused by the fact that using flags is not the best solution
in this case.

OK, would you share your ideas about better solution?


BTW:
Before the patch logs contained:
[sdap_get_generic_op_finished] (0x0080): LDAP sizelimit was exceeded, returning 
incomplete data
[sdap_get_generic_op_finished] (0x0400): Search result: Size limit 

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

[Lukas Slebodnik]

On (25/09/15 14:31), Pavel Reichl wrote:
>
>
>On 09/25/2015 01:59 PM, Lukas Slebodnik wrote:
>>On (25/09/15 13:30), Pavel Reichl wrote:
>>>Hello,
>>>
>>>please see simple patch attached.
>>>
>>>Thanks!
>>
>>>From 5717c6effcb0ac0cd16b4863adba088c9b1f0b90 Mon Sep 17 00:00:00 2001
>>>From: Pavel Reichl 
>>>Date: Fri, 25 Sep 2015 07:05:30 -0400
>>>Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
>>>
>>>Add new parameter 'flags' to sdap_get_generic_ext_send() which can be
>>>set to suppress warning about 'sizelimit exceeded'.
>>>
>>>Resolves:
>>>https://fedorahosted.org/sssd/ticket/2804
>>>---
>>>src/providers/ldap/sdap_async.c | 34 +-
>>>1 file changed, 25 insertions(+), 9 deletions(-)
>>>
>>>diff --git a/src/providers/ldap/sdap_async.c 
>>>b/src/providers/ldap/sdap_async.c
>>>index 
>>>97c9ea5df61a6516ca74bb73edc9a116b1266c71..1626d0dada1800aaf69681de13632b007866e779
>>> 100644
>>>--- a/src/providers/ldap/sdap_async.c
>>>+++ b/src/providers/ldap/sdap_async.c
>>>@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
>>> void *cb_data;
>>>
>>> bool allow_paging;
>>>+
>>>+unsigned int flags;
>>^^^
>> The int is redundant here
>> "unsigned int" and "unsigned" is the same.
>
>Indeed it's the same. But we use this form quite common. Do you want me to 
>file a ticket and assign it to you?
>
No,
we just needn't untroduce new versions.
The sort variant is already used in sssd
e.g.
src/tests/cmocka/test_nss_srv.c:static void 
mock_fill_group_with_members(unsigned members)
src/tests/cmocka/test_nss_srv.c:unsigned i;
src/tests/cmocka/test_nss_srv.c:unsigned i;
src/tests/cmocka/test_nss_srv.c:unsigned i;
src/tests/cmocka/test_sysdb_views.c: 
unsigned override_gid)
src/tests/cmocka/test_sysdb_views.c:   unsigned 
expected_override_gid)
src/tests/cmocka/test_sysdb_views.c:unsigned gid;

>git grep 'unsigned int' |  wc -l
>206
>
>
>>
>>>};
>>>
>>>static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
>>>@@ -1172,6 +1174,14 @@ static void sdap_get_generic_op_finished(struct 
>>>sdap_op *op,
>>>  struct sdap_msg *reply,
>>>  int error, void *pvt);
>>>
>>>+#define NO_FLAGS 0
>>>+#define DONT_WARN_SIZELIMIT_EXCEEDED 1
>>>+
>>>+static bool is_flag_set(unsigned int flags, unsigned int flag)
>>>+{
>>>+return (flags & flag) != 0;
>>>+}
>>>+
>>>static struct tevent_req *
>>>sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>>>   struct tevent_context *ev,
>>>@@ -1188,7 +1198,8 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>>>   int timeout,
>>>   bool allow_paging,
>>>   sdap_parse_cb parse_cb,
>>>-  void *cb_data)
>>>+  void *cb_data,
>>>+  unsigned int flags)
>>>{
>>> errno_t ret;
>>> struct sdap_get_generic_ext_state *state;
>>>@@ -1215,6 +1226,7 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>>> state->parse_cb = parse_cb;
>>> state->cb_data = cb_data;
>>> state->clientctrls = clientctrls;
>>>+state->flags = flags;
>>>
>>> if (state->sh == NULL || state->sh->ldap == NULL) {
>>> DEBUG(SSSDBG_CRIT_FAILURE,
>>>@@ -1500,8 +1512,11 @@ static void sdap_get_generic_op_finished(struct 
>>>sdap_op *op,
>>>
>>> if (result == LDAP_SIZELIMIT_EXCEEDED) {
>>> /* Try to return what we've got */
>>>-DEBUG(SSSDBG_MINOR_FAILURE,
>>>-  "LDAP sizelimit was exceeded, returning incomplete 
>>>data\n");
>>>+if (!is_flag_set(state->flags, DONT_WARN_SIZELIMIT_EXCEEDED)) {
>>The function is used just once and it does not 
>> significantly
>>improve readability of code; But it might be caused by too
>As negation is needed I believe it improves readability enough to advocate 
>it's existence.
>
>>long name of flag and many brackets + negation.
>>   if (state->flags & WARN_SIZELIMIT) might be easier to read.
>
>I don't like WARN_SIZELIMIT because setting any flag should not be required by 
>default and this flag would imply otherwise.
>
You still need to you "flag" NO_FLAGS. So the argument with default
should not be excuse for not improving readability of code.

BTW It might be caused by the fact that using flags is not the best solution
in this case.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Pavel Reichl]

Hello,

please see simple patch attached.

Thanks!
>From 5717c6effcb0ac0cd16b4863adba088c9b1f0b90 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send() which can be
set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
 src/providers/ldap/sdap_async.c | 34 +-
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 97c9ea5df61a6516ca74bb73edc9a116b1266c71..1626d0dada1800aaf69681de13632b007866e779 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
 void *cb_data;
 
 bool allow_paging;
+
+unsigned int flags;
 };
 
 static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,6 +1174,14 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
  struct sdap_msg *reply,
  int error, void *pvt);
 
+#define NO_FLAGS 0
+#define DONT_WARN_SIZELIMIT_EXCEEDED 1
+
+static bool is_flag_set(unsigned int flags, unsigned int flag)
+{
+return (flags & flag) != 0;
+}
+
 static struct tevent_req *
 sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   struct tevent_context *ev,
@@ -1188,7 +1198,8 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   int timeout,
   bool allow_paging,
   sdap_parse_cb parse_cb,
-  void *cb_data)
+  void *cb_data,
+  unsigned int flags)
 {
 errno_t ret;
 struct sdap_get_generic_ext_state *state;
@@ -1215,6 +1226,7 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
 state->parse_cb = parse_cb;
 state->cb_data = cb_data;
 state->clientctrls = clientctrls;
+state->flags = flags;
 
 if (state->sh == NULL || state->sh->ldap == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE,
@@ -1500,8 +1512,11 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
 
 if (result == LDAP_SIZELIMIT_EXCEEDED) {
 /* Try to return what we've got */
-DEBUG(SSSDBG_MINOR_FAILURE,
-  "LDAP sizelimit was exceeded, returning incomplete data\n");
+if (!is_flag_set(state->flags, DONT_WARN_SIZELIMIT_EXCEEDED)) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  "LDAP sizelimit was exceeded, "
+  "returning incomplete data\n");
+}
 } else if (result == LDAP_INAPPROPRIATE_MATCHING) {
 /* This error should only occur when we're testing for
  * specialized functionality like the ldap matching rule
@@ -1708,7 +1723,8 @@ struct tevent_req *sdap_get_and_parse_generic_send(TALLOC_CTX *memctx,
 subreq = sdap_get_generic_ext_send(state, ev, opts, sh, search_base,
scope, filter, attrs, false, NULL,
NULL, sizelimit, timeout, allow_paging,
-   sdap_get_and_parse_generic_parse_entry, state);
+   sdap_get_and_parse_generic_parse_entry,
+   state, NO_FLAGS);
 if (!subreq) {
 talloc_zfree(req);
 return NULL;
@@ -1919,7 +1935,7 @@ sdap_x_deref_search_send(TALLOC_CTX *memctx, struct tevent_context *ev,
filter, attrs,
false, state->ctrls, NULL, 0, timeout,
true, sdap_x_deref_parse_entry,
-   state);
+   state, NO_FLAGS);
 if (!subreq) {
 talloc_zfree(req);
 return NULL;
@@ -2143,7 +2159,7 @@ sdap_sd_search_send(TALLOC_CTX *memctx, struct tevent_context *ev,
LDAP_SCOPE_BASE, "(objectclass=*)", attrs,
false, state->ctrls, NULL, 0, timeout,
true, sdap_sd_search_parse_entry,
-   state);
+   state, NO_FLAGS);
 if (!subreq) {
 ret = EIO;
 goto fail;
@@ -2342,7 +2358,7 @@ sdap_asq_search_send(TALLOC_CTX *memctx, struct tevent_context *ev,
LDAP_SCOPE_BASE, NULL, attrs,
false, state->ctrls, NULL, 0, timeout,
true, sdap_asq_search_parse_entry,
-   state);
+   state, NO_FLAGS);
 

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

[Pavel Reichl]

On 09/25/2015 01:59 PM, Lukas Slebodnik wrote:

On (25/09/15 13:30), Pavel Reichl wrote:

Hello,

please see simple patch attached.

Thanks!



From 5717c6effcb0ac0cd16b4863adba088c9b1f0b90 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Fri, 25 Sep 2015 07:05:30 -0400
Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check

Add new parameter 'flags' to sdap_get_generic_ext_send() which can be
set to suppress warning about 'sizelimit exceeded'.

Resolves:
https://fedorahosted.org/sssd/ticket/2804
---
src/providers/ldap/sdap_async.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 
97c9ea5df61a6516ca74bb73edc9a116b1266c71..1626d0dada1800aaf69681de13632b007866e779
 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
 void *cb_data;

 bool allow_paging;
+
+unsigned int flags;

^^^
 The int is redundant here
 "unsigned int" and "unsigned" is the same.


Indeed it's the same. But we use this form quite common. Do you want me to file 
a ticket and assign it to you?

git grep 'unsigned int' |  wc -l
206





};

static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
@@ -1172,6 +1174,14 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,
  struct sdap_msg *reply,
  int error, void *pvt);

+#define NO_FLAGS 0
+#define DONT_WARN_SIZELIMIT_EXCEEDED 1
+
+static bool is_flag_set(unsigned int flags, unsigned int flag)
+{
+return (flags & flag) != 0;
+}
+
static struct tevent_req *
sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   struct tevent_context *ev,
@@ -1188,7 +1198,8 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
   int timeout,
   bool allow_paging,
   sdap_parse_cb parse_cb,
-  void *cb_data)
+  void *cb_data,
+  unsigned int flags)
{
 errno_t ret;
 struct sdap_get_generic_ext_state *state;
@@ -1215,6 +1226,7 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
 state->parse_cb = parse_cb;
 state->cb_data = cb_data;
 state->clientctrls = clientctrls;
+state->flags = flags;

 if (state->sh == NULL || state->sh->ldap == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE,
@@ -1500,8 +1512,11 @@ static void sdap_get_generic_op_finished(struct sdap_op 
*op,

 if (result == LDAP_SIZELIMIT_EXCEEDED) {
 /* Try to return what we've got */
-DEBUG(SSSDBG_MINOR_FAILURE,
-  "LDAP sizelimit was exceeded, returning incomplete data\n");
+if (!is_flag_set(state->flags, DONT_WARN_SIZELIMIT_EXCEEDED)) {

The function is used just once and it does not significantly
improve readability of code; But it might be caused by too

As negation is needed I believe it improves readability enough to advocate it's 
existence.


long name of flag and many brackets + negation.
   if (state->flags & WARN_SIZELIMIT) might be easier to read.


I don't like WARN_SIZELIMIT because setting any flag should not be required by 
default and this flag would imply otherwise.



The simillar patter is used also on other places.
src/sss_client/pam_sss.c:109:if (err & PAM_DATA_REPLACE) {
src/sss_client/pam_sss.c:1537:if (flags & FLAGS_USE_FIRST_PASS) {
src/sss_client/pam_sss.c:1546:if (flags & FLAGS_USE_2FA
src/sss_client/pam_sss.c:1561:if (flags & FLAGS_FORWARD_PASS) {
src/sss_client/pam_sss.c:1592:if (pam_flags & PAM_PRELIM_CHECK) {
...
and many other places

sh$ git grep -n if | grep " & " | wc -l
177

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

[Lukas Slebodnik]

On (25/09/15 13:30), Pavel Reichl wrote:
>Hello,
>
>please see simple patch attached.
>
>Thanks!

>From 5717c6effcb0ac0cd16b4863adba088c9b1f0b90 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl 
>Date: Fri, 25 Sep 2015 07:05:30 -0400
>Subject: [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
>
>Add new parameter 'flags' to sdap_get_generic_ext_send() which can be
>set to suppress warning about 'sizelimit exceeded'.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2804
>---
> src/providers/ldap/sdap_async.c | 34 +-
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
>diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
>index 
>97c9ea5df61a6516ca74bb73edc9a116b1266c71..1626d0dada1800aaf69681de13632b007866e779
> 100644
>--- a/src/providers/ldap/sdap_async.c
>+++ b/src/providers/ldap/sdap_async.c
>@@ -1164,6 +1164,8 @@ struct sdap_get_generic_ext_state {
> void *cb_data;
> 
> bool allow_paging;
>+
>+unsigned int flags;
   ^^^
The int is redundant here
"unsigned int" and "unsigned" is the same.

> };
> 
> static errno_t sdap_get_generic_ext_step(struct tevent_req *req);
>@@ -1172,6 +1174,14 @@ static void sdap_get_generic_op_finished(struct sdap_op 
>*op,
>  struct sdap_msg *reply,
>  int error, void *pvt);
> 
>+#define NO_FLAGS 0
>+#define DONT_WARN_SIZELIMIT_EXCEEDED 1
>+
>+static bool is_flag_set(unsigned int flags, unsigned int flag)
>+{
>+return (flags & flag) != 0;
>+}
>+
> static struct tevent_req *
> sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>   struct tevent_context *ev,
>@@ -1188,7 +1198,8 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>   int timeout,
>   bool allow_paging,
>   sdap_parse_cb parse_cb,
>-  void *cb_data)
>+  void *cb_data,
>+  unsigned int flags)
> {
> errno_t ret;
> struct sdap_get_generic_ext_state *state;
>@@ -1215,6 +1226,7 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> state->parse_cb = parse_cb;
> state->cb_data = cb_data;
> state->clientctrls = clientctrls;
>+state->flags = flags;
> 
> if (state->sh == NULL || state->sh->ldap == NULL) {
> DEBUG(SSSDBG_CRIT_FAILURE,
>@@ -1500,8 +1512,11 @@ static void sdap_get_generic_op_finished(struct sdap_op 
>*op,
> 
> if (result == LDAP_SIZELIMIT_EXCEEDED) {
> /* Try to return what we've got */
>-DEBUG(SSSDBG_MINOR_FAILURE,
>-  "LDAP sizelimit was exceeded, returning incomplete data\n");
>+if (!is_flag_set(state->flags, DONT_WARN_SIZELIMIT_EXCEEDED)) {
   The function is used just once and it does not significantly
   improve readability of code; But it might be caused by too
   long name of flag and many brackets + negation.
  if (state->flags & WARN_SIZELIMIT) might be easier to read.

The simillar patter is used also on other places.
src/sss_client/pam_sss.c:109:if (err & PAM_DATA_REPLACE) {
src/sss_client/pam_sss.c:1537:if (flags & FLAGS_USE_FIRST_PASS) {
src/sss_client/pam_sss.c:1546:if (flags & FLAGS_USE_2FA
src/sss_client/pam_sss.c:1561:if (flags & FLAGS_FORWARD_PASS) {
src/sss_client/pam_sss.c:1592:if (pam_flags & PAM_PRELIM_CHECK) {
...
and many other places

sh$ git grep -n if | grep " & " | wc -l
177

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel