[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