On 03/09/2015 02:49 PM, Tomas Babej wrote: > > On 03/06/2015 01:08 PM, Alexander Bokovoy wrote: >> On Thu, 05 Mar 2015, Sumit Bose wrote: >>> On Thu, Mar 05, 2015 at 09:16:36AM +0100, Sumit Bose wrote: >>>> On Wed, Mar 04, 2015 at 06:14:53PM +0100, Sumit Bose wrote: >>>> > On Wed, Mar 04, 2015 at 04:17:55PM +0200, Alexander Bokovoy wrote: >>>> > > On Mon, 02 Mar 2015, Sumit Bose wrote: >>>> > > >diff --git >>>> a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c >>>> b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c >>>> > > >index >>>> 20fdd62b20f28f5384cf83b8be5819f721c6c3db..84aeb28066f25f05a89d0c2d42e8b060e2399501 >>>> 100644 >>>> > > >--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c >>>> > > >+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c >>>> > > >@@ -49,6 +49,220 @@ >>>> > > > >>>> > > >#define MAX(a,b) (((a)>(b))?(a):(b)) >>>> > > >#define SSSD_DOMAIN_SEPARATOR '@' >>>> > > >+#define MAX_BUF (1024*1024*1024) >>>> > > >+ >>>> > > >+ >>>> > > >+ >>>> > > >+static int get_buffer(size_t *_buf_len, char **_buf) >>>> > > >+{ >>>> > > >+ long pw_max; >>>> > > >+ long gr_max; >>>> > > >+ size_t buf_len; >>>> > > >+ char *buf; >>>> > > >+ >>>> > > >+ pw_max = sysconf(_SC_GETPW_R_SIZE_MAX); >>>> > > >+ gr_max = sysconf(_SC_GETGR_R_SIZE_MAX); >>>> > > >+ >>>> > > >+ if (pw_max == -1 && gr_max == -1) { >>>> > > >+ buf_len = 16384; >>>> > > >+ } else { >>>> > > >+ buf_len = MAX(pw_max, gr_max); >>>> > > >+ } >>>> > > Here you'd get buf_len equal to 1024 by default on Linux which is too >>>> > > low for our use case. I think it would be beneficial to add one more >>>> > > MAX(buf_len, 16384): >>>> > > - if (pw_max == -1 && gr_max == -1) { >>>> > > - buf_len = 16384; >>>> > > - } else { >>>> > > - buf_len = MAX(pw_max, gr_max); >>>> > > - } >>>> > > + buf_len = MAX(16384, MAX(pw_max, gr_max)); >>>> > > >>>> > > with MAX(MAX(),..) you also get rid of if() statement as resulting >>>> > > rvalue would be guaranteed to be positive. >>>> > >>>> > done >>>> > >>>> > > >>>> > > The rest is going along the common lines but would it be better to >>>> > > allocate memory once per LDAP client request rather than always ask for >>>> > > it per each NSS call? You can guarantee a sequential use of the buffer >>>> > > within the LDAP client request processing so there is no problem with >>>> > > locks but having this memory re-allocated on subsequent >>>> > > getpwnam()/getpwuid()/... calls within the same request processing >>>> > > seems >>>> > > suboptimal to me. >>>> > >>>> > ok, makes sense, I moved get_buffer() back to the callers. >>>> > >>>> > New version attached. >>>> >>>> Please ignore this patch, I will send a revised version soon. >>> >>> Please find attached a revised version which properly reports missing >>> objects and out-of-memory cases and makes sure buf and buf_len are in >>> sync. >> ACK to patches 0135 and 0136. This concludes the review, thanks! >> > > Pushed to master: c15a407cbfaed163a933ab137eed16387efe25d2
As per https://fedorahosted.org/freeipa/ticket/4908, we will need this in 4.1.x also: Pushed to ipa-4-1: ec7a55a05647c4abad4c2a1bb5b5094f1e1eec55 Martin -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code