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

Pavel Reichl preichl at redhat.com
Tue Oct 13 16:36:14 UTC 2015



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

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


More information about the sssd-devel mailing list