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

Jakub Hrozek jhrozek at redhat.com
Sun Oct 11 09:06:47 UTC 2015


On Fri, Oct 09, 2015 at 08:02:08PM +0200, 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 :)

LGTM (not tested).


More information about the sssd-devel mailing list