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

Jakub Hrozek jhrozek at redhat.com
Fri Oct 9 12:00:52 UTC 2015


On Fri, Oct 09, 2015 at 01:56:22PM +0200, 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
> 
> > 
> > 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.

It would also be better if the flags definition was hexadecimal and
namespaced:
    #define SDAP_SRCH_FLG_PAGING              0x01
    #define SDAP_SRCH_FLG_SIZELIMIT_SILENT    0x02
    #define SDAP_SRCH_FLG_FOOBAR              0x04
    ...


More information about the sssd-devel mailing list