[SSSD] [PATCH] SDAP: rem warning - sizelimit exceeded in POSIX check
Jakub Hrozek
jhrozek at redhat.com
Fri Oct 9 11:56:22 UTC 2015
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
>
> I believe it's better to instruct directly sdap_get_generic_ext_send() not
> to print the warning. I understand that adding parameters to functions is
> not desired, but I think that in this case it's justified. I could even make
> parameter allow_paging to be part of the flag and thus don't increase the
> number of parameters at all.
If you still prefer your version, then I guess it would be better to
converge on one parameter to control the behaviour (but I wouldn't
change allow_paging in sdap_get_and_parse_generic_send)
>
> >
> >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.
>
> Yes, I believe your proposed solution would work, but I'm worried that it might be more robust change and it could be more error prone.
>
> Please reconsider attached patch.
>
> Thanks!
>
More information about the sssd-devel
mailing list