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

Pavel Reichl preichl at redhat.com
Tue Oct 13 16:32:34 UTC 2015



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


More information about the sssd-devel mailing list