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