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

Pavel Reichl preichl at redhat.com
Wed Oct 14 11:07:00 UTC 2015



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


More information about the sssd-devel mailing list