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.
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?
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel