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

Pavel Reichl preichl at redhat.com
Tue Sep 29 08:03:30 UTC 2015



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

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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SDAP-rem-warning-sizelimit-exceeded-in-POSIX-check.patch
Type: text/x-patch
Size: 6897 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150929/5ad6132b/attachment.bin>


More information about the sssd-devel mailing list