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 preichl@redhat.com 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
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 preichl@redhat.com 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
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 preichl@redhat.com 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
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 preichl@redhat.com 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 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
I believe this is OK as purpose of this patch was to remove only too loud warning or do we want to get rid of the warning completely?
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 preichl@redhat.com 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.
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 preichl@redhat.com 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!
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 preichl@redhat.com 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 robust change and it could be more error prone.
Please reconsider attached patch.
Thanks!
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 preichl@redhat.com 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_SILENT 0x02 #define SDAP_SRCH_FLG_FOOBAR 0x04 ...
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 preichl@redhat.com 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
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 preichl@redhat.com 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 :)
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 preichl@redhat.com 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 :)
LGTM (not tested).
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 preichl@redhat.com 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?= mzidek@redhat.com Date: Fri, 9 Oct 2015 19:24:40 +0200 Subject: [PATCH] sdap: Ignore exceeding sizelimit during posix check
Esceeding sizelimit during posix check is expected and should not be logged.
Resolves: https://fedorahosted.org/sssd/ticket/2804
src/providers/ldap/sdap_async.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index b81431f..ce65fc9 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -1148,6 +1148,7 @@ struct sdap_get_generic_ext_state { int timeout; int attrsonly; int sizelimit;
bool log_sizelimit_exceeded;
struct sdap_op *op;
@@ -1208,7 +1209,12 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx, state->attrs = attrs; state->attrsonly = attrsonly; state->op = NULL;
- state->sizelimit = sizelimit;
- /* Negative value of sizelimit means that we do not want
* to print debug message if ldap sizelimit is exceeded.
* This can be used if we only want to check if attributes
* exist, but we do not want to fetch them all. */
- state->sizelimit = abs(sizelimit);
- state->log_sizelimit_exceeded = sizelimit >= 0;
Jakub's solution is the cleanest way from technical point of view. If we do not want to handle "size limit exceeded" in the same way for all use cases then the returned error code is the best solution for such low level functions as sdap_get_generic_ext_send.
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. Our use case is similar. We do not know how to handle size limit exceeded. Because in some case we want to log a message and in other cases we want to ignore it.
LS
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.
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.
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 preichl@redhat.com > 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?= mzidek@redhat.com Date: Fri, 9 Oct 2015 19:24:40 +0200 Subject: [PATCH] sdap: Ignore exceeding sizelimit during posix check
Esceeding sizelimit during posix check is expected and should not be logged.
Resolves: https://fedorahosted.org/sssd/ticket/2804
src/providers/ldap/sdap_async.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index b81431f..ce65fc9 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -1148,6 +1148,7 @@ struct sdap_get_generic_ext_state { int timeout; int attrsonly; int sizelimit;
bool log_sizelimit_exceeded;
struct sdap_op *op;
@@ -1208,7 +1209,12 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx, state->attrs = attrs; state->attrsonly = attrsonly; state->op = NULL;
- state->sizelimit = sizelimit;
- /* Negative value of sizelimit means that we do not want
* to print debug message if ldap sizelimit is exceeded.
* This can be used if we only want to check if attributes
* exist, but we do not want to fetch them all. */
- state->sizelimit = abs(sizelimit);
- state->log_sizelimit_exceeded = sizelimit >= 0;
Jakub's solution is the cleanest way from technical point of view. If we do not want to handle "size limit exceeded" in the same way for all use cases then the returned error code is the best solution for such low level functions as sdap_get_generic_ext_send.
I'm sorry but I can't agree with this. IMHO taking part of a function and copy&pasting it to other places of code can hardly be the cleanest solution. Also updating recv function is not necessary with other solutions.
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. Our use case is similar. We do not know how to handle size limit exceeded. Because in some case we want to log a message and in other cases we want to ignore it.
Well, Lukas this is nicely put. But on the other hand C is not a high level language and we should use it's native features and not to try emulate features from higher languages. Bit mask flags are native and commonly used feature of C.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 preichl@redhat.com 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 sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 preichl@redhat.com 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 LDAP_SIZELIMIT_EXCEEDED it most probably does not want that the data is processed as well. In this case I think ti would make more sense to use another bit in the flags option to tell sdap_get_generic_op_finished() to not ignore LDAP_SIZELIMIT_EXCEEDED but return ERR_SIZELIMIT_EXCEEDED immediately without further processing of the data.
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?
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 preichl@redhat.com 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 LDAP_SIZELIMIT_EXCEEDED it most probably does not want that the data is processed as well. In this case I think ti would make more sense to use another bit in the flags option to tell sdap_get_generic_op_finished() to not ignore LDAP_SIZELIMIT_EXCEEDED but return ERR_SIZELIMIT_EXCEEDED immediately without further processing of the data.
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..
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 preichl@redhat.com 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 LDAP_SIZELIMIT_EXCEEDED it most probably does not want that the data is processed as well. In this case I think ti would make more sense to use another bit in the flags option to tell sdap_get_generic_op_finished() to not ignore LDAP_SIZELIMIT_EXCEEDED but return ERR_SIZELIMIT_EXCEEDED immediately without further processing of the data.
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.
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!
On Wed, Oct 14, 2015 at 04:09:25PM +0200, Pavel Reichl wrote:
On 10/14/2015 01:07 PM, Pavel Reichl wrote:
On 10/14/2015 01:01 PM, Jakub Hrozek wrote:
On Wed, Oct 14, 2015 at 12:51:39PM +0200, Sumit Bose wrote:
[snip]
So far I liked the flags attribute which controls the behaviour of sdap_get_generic_ext_send() best (and I agree that allow_paging should be include in the flags, but only for sdap_get_generic_ext_send() not the "higher" level calls). Mainly because it scales, i.e. it can be used in future to control sdap_get_generic_ext_send() even more.
[snip]
If you think this is all too much for the issue at hand (ignoring a debug message) especially for versions that are already released what about always ignoring the message (or only displaying with SSSDBG_TRACE_ALL) if state->sizelimit != 0, i.e. explicitly set?
No, I don't really mind, I think we already spent too much time discussing this simple patch.
If you like the flags best, let's push the flags.. _______________________________________________
OK, I'll send updated patch reflecting comments relating 'flags' patch. _______________________________________________
Please see updated patch set. 1st patch adds flag to control silencing of warning 2nd patch makes allow_paging part of flag 3rd patch removes unused parameter 'attrsonly' from function sdap_get_and_parse_generic_send() 4rd patch makes attrsonly in sdap_get_generic_ext_state to be part of flag
If any of patches 2,3,4 is too controversial feel free to ignore it, I can send it later in separate thread.
Thanks!
Thank you for the patches.
I have two things I would like to discuss
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index b81431f79f21755469bb9ff123d695a2a166e353..cbf199e6d524a3aa659ef040937b26571ab63735 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -1164,6 +1164,7 @@ struct sdap_get_generic_ext_state { void *cb_data;
bool allow_paging;
- bool sizelimit_silent;
What about putting the flags directly in the state and check them only where they might apply. I think this will scale better because there is no need to add a boolean for every new flag value and does not need the (mostly useless?) conversion from flag to bool at the start of sdap_get_generic_ext_send().
};
static errno_t sdap_get_generic_ext_step(struct tevent_req *req); @@ -1172,6 +1173,9 @@ static void sdap_get_generic_op_finished(struct sdap_op *op, struct sdap_msg *reply, int error, void *pvt);
+/* Be silent about exceeded size limit */ +#define SDAP_SRCH_FLG_SIZELIMIT_SILENT 0x01
I wonder if the (1 << 0) style notation used e.g. for flags in the pam_sss module or for SBUS_PROPERTY_* would be less error-prone to define flag values?
Otherwise the patches look good and I think it is a good idea to include attrsonly here as well.
I didn't run any tests so far because I would like to hear your option about the two suggestions from above first.
bye, Sumit
On 10/15/2015 11:24 AM, Sumit Bose wrote:
On Wed, Oct 14, 2015 at 04:09:25PM +0200, Pavel Reichl wrote:
On 10/14/2015 01:07 PM, Pavel Reichl wrote:
On 10/14/2015 01:01 PM, Jakub Hrozek wrote:
On Wed, Oct 14, 2015 at 12:51:39PM +0200, Sumit Bose wrote:
[snip]
So far I liked the flags attribute which controls the behaviour of sdap_get_generic_ext_send() best (and I agree that allow_paging should be include in the flags, but only for sdap_get_generic_ext_send() not the "higher" level calls). Mainly because it scales, i.e. it can be used in future to control sdap_get_generic_ext_send() even more.
[snip]
If you think this is all too much for the issue at hand (ignoring a debug message) especially for versions that are already released what about always ignoring the message (or only displaying with SSSDBG_TRACE_ALL) if state->sizelimit != 0, i.e. explicitly set?
No, I don't really mind, I think we already spent too much time discussing this simple patch.
If you like the flags best, let's push the flags.. _______________________________________________
OK, I'll send updated patch reflecting comments relating 'flags' patch. _______________________________________________
Please see updated patch set. 1st patch adds flag to control silencing of warning 2nd patch makes allow_paging part of flag 3rd patch removes unused parameter 'attrsonly' from function sdap_get_and_parse_generic_send() 4rd patch makes attrsonly in sdap_get_generic_ext_state to be part of flag
If any of patches 2,3,4 is too controversial feel free to ignore it, I can send it later in separate thread.
Thanks!
Thank you for the patches.
I have two things I would like to discuss
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index b81431f79f21755469bb9ff123d695a2a166e353..cbf199e6d524a3aa659ef040937b26571ab63735 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -1164,6 +1164,7 @@ struct sdap_get_generic_ext_state { void *cb_data;
bool allow_paging;
- bool sizelimit_silent;
What about putting the flags directly in the state and check them only where they might apply. I think this will scale better because there is no need to add a boolean for every new flag value and does not need the (mostly useless?) conversion from flag to bool at the start of sdap_get_generic_ext_send().
Yes, that was my idea too. Then while working on second patch I noticed that allow_paging is not set only by value of allow_paging argument but depends on scope as well.
if (scope == LDAP_SCOPE_BASE) { state->allow_paging = false; } else { state->allow_paging = flags & SDAP_SRCH_FLG_PAGING; }
I'm not sure how to handle this: 1) remove boolean (allow_paging, sizelimit_silent, ...) fields from state altogether and use flags field for writing and reading.
- if (scope == LDAP_SCOPE_BASE) { - state->allow_paging = false; - } else { - state->allow_paging = flags & SDAP_SRCH_FLG_PAGING; - } + if (scope == LDAP_SCOPE_BASE) { + state->flags &= ~SDAP_SRCH_FLG_PAGING + } 2) keep boolean field for allow_paging only? 3) keep boolean fields for all values set by flags parameter and do not store flags as a field in state.
1) and 3) is fine by me, I don't like 2) because it's slightly inconsistent in the sense that allow_paging can be read from flags and has its own boolean field (both might have a different value).
};
static errno_t sdap_get_generic_ext_step(struct tevent_req *req); @@ -1172,6 +1173,9 @@ static void sdap_get_generic_op_finished(struct sdap_op *op, struct sdap_msg *reply, int error, void *pvt);
+/* Be silent about exceeded size limit */ +#define SDAP_SRCH_FLG_SIZELIMIT_SILENT 0x01
I wonder if the (1 << 0) style notation used e.g. for flags in the pam_sss module or for SBUS_PROPERTY_* would be less error-prone to define flag values?
Thanks for the idea, I'll add this change to the patch set.
Otherwise the patches look good and I think it is a good idea to include attrsonly here as well.
I didn't run any tests so far because I would like to hear your option about the two suggestions from above first.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@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!
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:
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
- }
keep boolean field for allow_paging only?
keep boolean fields for all values set by flags parameter and do not store flags as a field in state.
and 3) is fine by me, I don't like 2) because it's slightly inconsistent in the sense that allow_paging can be read from flags and has its own boolean field (both might have a different value).
The check was added to fix https://fedorahosted.org/sssd/ticket/1202 was we should disable paging here. I think it makes most sense to do 1) nevertheless I would add a debug message here because in theory this case should never happen in the first place because if makes no sense for a caller to use a base search with paging and hence it should be fixed on the caller side as well.
Maybe we want to be even more radical in a later release and just return an error in this case. But I would start with the debug message to see if there might be some legitimate cases where it is not clear how the parameters are set and sdap_get_generic_ext_send() is the best place to sort it out.
};
static errno_t sdap_get_generic_ext_step(struct tevent_req *req); @@ -1172,6 +1173,9 @@ static void sdap_get_generic_op_finished(struct sdap_op *op, struct sdap_msg *reply, int error, void *pvt);
+/* Be silent about exceeded size limit */ +#define SDAP_SRCH_FLG_SIZELIMIT_SILENT 0x01
I wonder if the (1 << 0) style notation used e.g. for flags in the pam_sss module or for SBUS_PROPERTY_* would be less error-prone to define flag values?
Thanks for the idea, I'll add this change to the patch set.
Otherwise the patches look good and I think it is a good idea to include attrsonly here as well.
I didn't run any tests so far because I would like to hear your option about the two suggestions from above first.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sumit what do you think about patch 3? I noticed that also **serverctrls and **clientctrls are unused, should I remove all 3 parematers or should I pass them instead? I checked the code and there would be no difference in the actual parameters passed to sdap_get_generic_ext_send().
no, please keep them. sdap_get_generic_ext_send() is our wrapper for ldap_search_ext() and should make all option ldap_search_ext() has available to callers in SSSD.
It looks we only do not set the timeout parameter in ldap_search_ext(). This is ok, since it only controls the server-side timeout and the timeout is handled separately by the sdap_op scheme. Nevertheless it might make sense to talk to LDAP server developers to see if it would help to set the option as well. I'm thinking of cases where SSSD might trigger a long running operation on the server but drops the request after a few seconds on the client. The server is then still busy preparing the response for the client and when it is ready it is not needed anymore. So sending the timeout might help to reduce the server load? But of course this should not be part of the current set of patches and should be handled independently.
bye, Sumit
Thanks! _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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:
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
- }
keep boolean field for allow_paging only?
keep boolean fields for all values set by flags parameter and do not store flags as a field in state.
and 3) is fine by me, I don't like 2) because it's slightly inconsistent in the sense that allow_paging can be read from flags and has its own boolean field (both might have a different value).
The check was added to fix https://fedorahosted.org/sssd/ticket/1202 was we should disable paging here. I think it makes most sense to do 1) nevertheless I would add a debug message here because in theory this case should never happen in the first place because if makes no sense for a caller to use a base search with paging and hence it should be fixed on the caller side as well.
We also set allow_paging = true if some controls are to be used without checking the scope. I added warning for that case (please let me know if it's useless).
Maybe we want to be even more radical in a later release and just return an error in this case. But I would start with the debug message to see if there might be some legitimate cases where it is not clear how the parameters are set and sdap_get_generic_ext_send() is the best place to sort it out.
};
static errno_t sdap_get_generic_ext_step(struct tevent_req *req); @@ -1172,6 +1173,9 @@ static void sdap_get_generic_op_finished(struct sdap_op *op, struct sdap_msg *reply, int error, void *pvt);
+/* Be silent about exceeded size limit */ +#define SDAP_SRCH_FLG_SIZELIMIT_SILENT 0x01
I wonder if the (1 << 0) style notation used e.g. for flags in the pam_sss module or for SBUS_PROPERTY_* would be less error-prone to define flag values?
Thanks for the idea, I'll add this change to the patch set.
Otherwise the patches look good and I think it is a good idea to include attrsonly here as well.
I didn't run any tests so far because I would like to hear your option about the two suggestions from above first.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@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
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:
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
- }
keep boolean field for allow_paging only?
keep boolean fields for all values set by flags parameter and do not store flags as a field in state.
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_SILENT 0x01
I wonder if the (1 << 0) style notation used e.g. for flags in the pam_sss module or for SBUS_PROPERTY_* would be less error-prone to define flag values?
Thanks for the idea, I'll add this change to the patch set.
Otherwise the patches look good and I think it is a good idea to include attrsonly here as well.
I didn't run any tests so far because I would like to hear your option about the two suggestions from above first.
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@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.
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 preichl@redhat.com 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 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index b81431f79f21755469bb9ff123d695a2a166e353..77572d14c759805985aa74825c694deee64ae237 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,15 @@ 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");
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.
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
bye, Sumit
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.
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
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
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@lists.fedorahosted.org