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

Lukas Slebodnik lslebodn at redhat.com
Tue Sep 29 06:46:11 UTC 2015


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


More information about the sssd-devel mailing list