Ticket #47314 - Winsync should support range retrieval

Bug description: AD returns up to MaxValRange number of multi-
valued attribute values in one search. If more attribute values
exist, subtype ";range=0-(MaxValRange?-1)" is added to the type.
AD Client (DS in this case) has to repeat the search with
";range=MaxValRange?-*" then ";range=(2*MaxValRange?)-*" and so
on until the values with the subtype ";range=low-*" are returned.
Windows Sync plugin did not support the range search.

Fix description: This patch implements the range search for the
multi-valued attribute.

In addition,
1) For the search request, AD returns an attribute with no values
if it was deleted on the AD side even if the delete is one of the
multi-valued attributes. Windows Sync code was always adding the
attribute to the entry's e_deleted_attr list. This patch stops it
if there are more attributes having the same type existing in the
entry.

2) slapi_entry_attr_get_* APIs call slapi_entry_attr_find and set
the return value from the function to the value to return from the
APIs. slapi_entry_attr_find returns -1 if it fails to find the
attribute, which was returned if the type did not exist in the
entry.  This patch stops it and let slapi_entry_attr_get_* APIs
return the default return value 0 in case slapi_entry_attr_find
fails to find the type.

Reviewed by Rich (Thank you!!)

https://fedorahosted.org/389/ticket/47314

ldap/servers/plugins/posix-winsync/posix-winsync.c

@@ -773,12 +773,19 @@ posix_winsync_pre_ds_mod_user_cb(void *cbdata, const Slapi_Entry *rawentry, Slap
     windows_attribute_map *attr_map = user_attribute_map;
     PRBool posixval = PR_TRUE;
 
-    if (posix_winsync_config_get_msSFUSchema())
-        attr_map = user_mssfu_attribute_map;
-
     slapi_log_error(SLAPI_LOG_PLUGIN, posix_winsync_plugin_name,
                     "--> _pre_ds_mod_user_cb -- begin\n");
 
+    if ((NULL == rawentry) || (NULL == ad_entry) || (NULL == ds_entry)) {
+        slapi_log_error(SLAPI_LOG_PLUGIN, posix_winsync_plugin_name,
+                        "<-- _pre_ds_mod_user_cb -- Empty %s entry.\n",
+                        (NULL==rawentry)?"rawentry":(NULL==ad_entry)?"ad entry":"ds entry");
+        return;
+    }
+
+    if (posix_winsync_config_get_msSFUSchema())
+        attr_map = user_mssfu_attribute_map;
+
     /* check all of the required attributes are in the ad_entry:
      * MUST (cn $ uid $ uidNumber $ gidNumber $ homeDirectory).
      * If any of the required attributes are missing, drop them before adding

ldap/servers/plugins/replication/windows_connection.c

@@ -523,14 +523,27 @@ windows_perform_operation(Repl_Connection *conn, int optype, const char *dn,
 }
 
 /* Copied from the chaining backend*/
+/* 
+ * exattrs: exceeded attribute list
+ * If attr value pair exceeds MaxValRange, AD returns, e.g.,
+ *   <attr>;range=0-<maxValRange-1>: <value>
+ * We need to repeat the search with "<attr>;range=1500-*"
+ * until it returns 
+ *   <attr>;range=<num>-*
+ */
 static Slapi_Entry * 
-windows_LDAPMessage2Entry(Repl_Connection *conn, LDAPMessage * msg, int attrsonly) {
+windows_LDAPMessage2Entry(Slapi_Entry *e, Repl_Connection *conn,
+                          LDAPMessage * msg, int attrsonly, char ***exattrs)
+{
 
 	Slapi_Entry *rawentry = NULL;
-	Slapi_Entry *e = NULL;
 	char *a = NULL;
 	BerElement * ber = NULL;
 	LDAP *ld = conn->ld;
+	int exattrlen = 0;
+	int exattridx = 0;
+	char **deletedattrs = NULL;
+	char **dap;
 
 	windows_private_set_raw_entry(conn->agmt, NULL); /* clear it first */
 
@@ -543,9 +556,10 @@ windows_LDAPMessage2Entry(Repl_Connection *conn, LDAPMessage * msg, int attrsonl
 	 * attribute type and values ARE allocated
 	 */
 
-	e = slapi_entry_alloc();
-	if ( e == NULL ) return NULL;
-	slapi_entry_set_dn( e, ldap_get_dn( ld, msg ) );
+	if (NULL == e) {
+		e = slapi_entry_alloc();
+		slapi_entry_set_dn( e, ldap_get_dn( ld, msg ) );
+	}
 	rawentry = slapi_entry_alloc();
 	if ( rawentry == NULL ) {
 		slapi_entry_free(e);
@@ -575,13 +589,59 @@ windows_LDAPMessage2Entry(Repl_Connection *conn, LDAPMessage * msg, int attrsonl
 				slapi_entry_add_value(e, a, (Slapi_Value *)NULL);
 			} else 
 			{
+#define SUBTYPERANGE "range="
 				char *type_to_use = NULL;
+				char *dupa = slapi_ch_strdup(a);
+				char *newa = NULL; /* dup of 'a' with next range */
+				char *p, *wp, *pp; /* work pointers */
+				char *iter;
+				int high = 0;
+				int sizea = strlen(a) + 2;
+				/* handling subtype(s) */
+				ldap_utf8strtok_r(dupa, ";", &iter); /* primry type */
+				p = ldap_utf8strtok_r(NULL, ";", &iter); /* subtype, if any */
+				while (p) {
+					if (0 == strncasecmp(p, SUBTYPERANGE, sizeof(SUBTYPERANGE) - 1)) {
+						/* get rid of range */
+						if (!newa) { /* first time for range= */
+							/* Cannot use strdup,
+							 * since 'a' could be "<attr>;range=0-9";
+							 * then newa is <attr>;10-*; newa is 1 char longer than a. */
+							newa = (char *)slapi_ch_malloc(sizea);
+							PR_snprintf(newa, sizea, "%s", a);
+							*(newa + (p - dupa) - 1) = '\0';
+						}
+						/* get the last count (high + 1) */
+						/* range=low-high */
+						pp = strchr(p, '-');
+						if (*++pp == '*') {
+							high = 0; /* high is *; done! */
+						} else {
+							high = strtol(pp, &p, 10);
+							if (high > 0) {
+								/* next low == high + 1 */
+								high++;
+							}
+						}
+					} else { /* subtype except "range=low-high" */
+						if (newa) {
+							int sizenewa = strlen(newa);
+							/* range= appeared before, copy this subtype */
+							wp = newa + sizenewa;
+							/* append ;<subtype> */
+							PR_snprintf(wp, sizea - sizenewa, ";%s", p);
+						}
+					}
+					p = ldap_utf8strtok_r(NULL, ";", &iter);
+				}
+				slapi_ch_free_string(&dupa);
+
 				/* Work around the fact that we alias street and streetaddress, while Microsoft do not */
-				if (0 == strcasecmp(a,"streetaddress")) 
-				{
+				if (0 == strcasecmp(a, "streetaddress")) {
 					type_to_use = FAKE_STREET_ATTR_NAME;
-				} else
-				{
+				} else if (newa) {
+					type_to_use = newa;
+				} else {
 					type_to_use = a;
 				}
 
@@ -591,13 +651,38 @@ windows_LDAPMessage2Entry(Repl_Connection *conn, LDAPMessage * msg, int attrsonl
 				if (aVal == NULL) {
 					/* Windows will send us an attribute with no values if it was deleted
 					 * on the AD side.  Add this attribute to the deleted attributes list */
-					Slapi_Attr *attr = slapi_attr_new();
-					slapi_attr_init(attr, type_to_use);
-					entry_add_deleted_attribute_wsi(e, attr);
+					/* Set it to the deleted attribute list only if the attribute does
+					 * not exist in the entry.  For the multi-valued attribute (e.g.,
+					 * member), if there are multiple member attributes in an entry, 
+					 * and one of them is deleted, this no value member is sent.  But
+					 * if there are more member attributes in the entry, we should not
+					 * set member to the deleted attribute. */
+					if (!charray_inlist(deletedattrs, type_to_use)) {
+						charray_add(&deletedattrs, slapi_ch_strdup(type_to_use));
+					}
 				} else { 
-					slapi_entry_add_values( e, type_to_use, aVal);
+					slapi_entry_add_values(e, type_to_use, aVal);
 				} 
-                
+
+				/* if the addr for exattrs is given and next range retrieval is needed */
+				if (exattrs && (high > 0)) {
+					if (exattrlen == exattridx) {
+						if (!*exattrs) {
+							exattrlen = 4;
+							exattridx = 0;
+							*exattrs = (char **)slapi_ch_calloc(exattrlen, sizeof(char *));
+						} else {
+							*exattrs = (char **)slapi_ch_realloc((char *)*exattrs, exattrlen * 2 * sizeof(char *));
+							memset(*exattrs + exattrlen, '\0', exattrlen * sizeof(char *));
+							exattrlen *= 2;
+						}
+						PR_snprintf(newa + strlen(newa), strlen(a) + 2 - strlen(newa),
+						                          ";%s%d-*", SUBTYPERANGE, high);
+						(*exattrs)[exattridx++] = newa;
+					}
+				} else if (newa) {
+					slapi_ch_free_string(&newa);
+				}
 			}
 		}
 		ldap_memfree(a);
@@ -607,6 +692,18 @@ windows_LDAPMessage2Entry(Repl_Connection *conn, LDAPMessage * msg, int attrsonl
 	{
         ber_free( ber, 0 );
 	}
+	/* Windows will send us an attribute with no values if it was deleted
+	 * on the AD side.  Add this attribute to the deleted attributes list */
+	/* Set to e_deleted_attrs only if there is no attribute of the type. */
+	for (dap = deletedattrs; dap && *dap; dap++) {
+		Slapi_Attr *attr = NULL;
+		if (slapi_entry_attr_find(e, *dap, &attr)) { /* not found */
+			attr = slapi_attr_new();
+			slapi_attr_init(attr, *dap);
+			entry_add_deleted_attribute_wsi(e, attr);
+		}
+	}
+	charray_free(deletedattrs);
 
 	windows_private_set_raw_entry(conn->agmt, rawentry); /* windows private now owns rawentry */
 
@@ -643,6 +740,7 @@ windows_search_entry_ext(Repl_Connection *conn, char* searchbase, char *filter,
 		char *searchbase_copy = slapi_ch_strdup(searchbase);
 		char *filter_copy = slapi_ch_strdup(filter);
 		char **attrs = NULL;
+		char **exattrs = NULL;
 		LDAPControl **serverctrls_copy = NULL;
 
 		slapi_add_controls(&serverctrls_copy, serverctrls, 1 /* make a copy we can free */);
@@ -651,7 +749,7 @@ windows_search_entry_ext(Repl_Connection *conn, char* searchbase, char *filter,
 
 		winsync_plugin_call_pre_ad_search_cb(conn->agmt, NULL, &searchbase_copy, &scope, &filter_copy,
 											 &attrs, &serverctrls_copy);
-		
+next:
 		ldap_rc = ldap_search_ext_s(conn->ld, searchbase_copy, scope,
 			filter_copy, attrs, 0 /* attrsonly */,
 			serverctrls_copy , NULL /* client controls */,
@@ -665,16 +763,8 @@ windows_search_entry_ext(Repl_Connection *conn, char* searchbase, char *filter,
 							ldap_err2string(ldap_rc));
 		}
 
-		slapi_ch_free_string(&searchbase_copy);
-		slapi_ch_free_string(&filter_copy);
 		slapi_ch_array_free(attrs);
 		attrs = NULL;
-		ldap_controls_free(serverctrls_copy);
-		serverctrls_copy = NULL;
-
-		/* clear it here in case the search fails and
-		   we are left with a bogus old entry */
-		windows_private_set_raw_entry(conn->agmt, NULL);
 		if (LDAP_SUCCESS == ldap_rc)
 		{
 			LDAPMessage *message = ldap_first_entry(conn->ld, res);
@@ -690,9 +780,18 @@ windows_search_entry_ext(Repl_Connection *conn, char* searchbase, char *filter,
                                    nummessages, numentries, numreferences );
 			}
 
-			if (NULL != entry)
-			{
-				*entry = windows_LDAPMessage2Entry(conn,message,0);
+			if (entry) {
+				exattrs = NULL;
+				*entry = windows_LDAPMessage2Entry(*entry, conn, message, 0, &exattrs);
+				if (exattrs) {
+					/* some attribute returned "<attr>;range=low-high" */
+					attrs = exattrs;
+					if (res) {
+						ldap_msgfree(res);
+						res = NULL;
+					}
+					goto next;
+				}
 			}
 			/* See if there are any more entries : if so then that's an error
 			 * but we still need to get them to avoid gumming up the connection
@@ -710,6 +809,14 @@ windows_search_entry_ext(Repl_Connection *conn, char* searchbase, char *filter,
 		{
 			return_value = CONN_OPERATION_FAILED;
 		}
+		slapi_ch_free_string(&searchbase_copy);
+		slapi_ch_free_string(&filter_copy);
+		ldap_controls_free(serverctrls_copy);
+		serverctrls_copy = NULL;
+
+		/* clear it here in case the search fails and
+		   we are left with a bogus old entry */
+		windows_private_set_raw_entry(conn->agmt, NULL);
 		conn->last_ldap_error = ldap_rc;
 		if (NULL != res)
 		{
@@ -745,6 +852,7 @@ send_dirsync_search(Repl_Connection *conn)
 		/* need to strip the dn down to dc= */
 		const char *old_dn = slapi_sdn_get_ndn( windows_private_get_windows_subtree(conn->agmt) );
 		char *dn = slapi_ch_strdup(strstr(old_dn, "dc="));
+		char **exattrs = NULL;
 
 		if (conn->supports_dirsync == 0)
 		{
@@ -764,6 +872,9 @@ send_dirsync_search(Repl_Connection *conn)
 
 		winsync_plugin_call_dirsync_search_params_cb(conn->agmt, old_dn, &dn, &scope, &filter,
 													 &attrs, &server_controls);
+		exattrs = windows_private_get_range_attrs(conn->agmt);
+		charray_merge(&attrs, exattrs, 0 /* pass in */);
+		slapi_ch_free((void **)&exattrs); /* strings are passed in */
 
 		LDAPDebug( LDAP_DEBUG_REPL, "Sending dirsync search request\n", 0, 0, 0 );
 
@@ -924,10 +1035,20 @@ Slapi_Entry * windows_conn_get_search_result(Repl_Connection *conn)
 			{
 				if (( dn = ldap_get_dn( conn->ld, res )) != NULL ) 
 				{
+					char **exattrs = NULL;
 					slapi_log_error(SLAPI_LOG_REPL, windows_repl_plugin_name,"received entry from dirsync: %s\n", dn);
 					lm = ldap_first_entry( conn->ld, res );			
-					e = windows_LDAPMessage2Entry(conn,lm,0);
+					e = windows_private_get_curr_entry(conn->agmt); /* if range search, e != NULL */
+					e = windows_LDAPMessage2Entry(e, conn, lm, 0, &exattrs);
 					ldap_memfree(dn);
+					if (exattrs) {
+						/* some attribute returned "<attr>;range=low-high" */
+						windows_private_set_curr_entry(conn->agmt, e);
+						windows_private_set_range_attrs(conn->agmt, exattrs);
+					} else {
+						windows_private_set_curr_entry(conn->agmt, NULL);
+						windows_private_set_range_attrs(conn->agmt, NULL);
+					}
 				}
 			}
 			break;

ldap/servers/plugins/replication/windows_private.c

@@ -77,6 +77,8 @@ struct windowsprivate {
   time_t sync_interval; /* how often to run the dirsync search, in seconds */
   int one_way; /* Indicates if this is a one-way agreement and which direction it is */
   int move_action; /* Indicates what to do with DS entry if AD entry is moved out of scope */
+  Slapi_Entry *curr_entry; /* entry being retrieved; used for the range retrieval */
+  char **range_attrs; /* next attributes for the range retrieval */
 };
 
 static void windows_private_set_windows_domain(const Repl_Agmt *ra, char *domain);
@@ -1170,6 +1172,76 @@ windows_private_set_move_action(const Repl_Agmt *ra, int value)
 	LDAPDebug0Args( LDAP_DEBUG_TRACE, "<= windows_private_set_move_action\n" );
 }
 
+/* Get entry being retrieved; used for the range retrieval */
+Slapi_Entry *
+windows_private_get_curr_entry(const Repl_Agmt *ra)
+{
+	Dirsync_Private *dp;
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "=> windows_private_get_curr_entry\n" );
+
+	PR_ASSERT(ra);
+
+	dp = (Dirsync_Private *) agmt_get_priv(ra);
+	PR_ASSERT (dp);
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "<= windows_private_get_curr_entry\n" );
+
+	return dp->curr_entry;	
+}
+
+/* Set entry being retrieved; used for the range retrieval */
+void
+windows_private_set_curr_entry(const Repl_Agmt *ra, Slapi_Entry *e)
+{
+	Dirsync_Private *dp;
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "=> windows_private_set_curr_entry\n" );
+
+	PR_ASSERT(ra);
+
+	dp = (Dirsync_Private *) agmt_get_priv(ra);
+	PR_ASSERT (dp);
+	dp->curr_entry = e;
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "<= windows_private_set_curr_entry\n" );
+}
+
+/* Get next range retrieval attributes */
+char **
+windows_private_get_range_attrs(const Repl_Agmt *ra)
+{
+	Dirsync_Private *dp;
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "=> windows_private_get_range_attrs\n" );
+
+	PR_ASSERT(ra);
+
+	dp = (Dirsync_Private *) agmt_get_priv(ra);
+	PR_ASSERT (dp);
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "<= windows_private_get_range_attrs\n" );
+
+	return dp->range_attrs;	
+}
+
+/* Set next range retrieval attributes */
+void
+windows_private_set_range_attrs(const Repl_Agmt *ra, char **attrs)
+{
+	Dirsync_Private *dp;
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "=> windows_private_set_move_action\n" );
+
+	PR_ASSERT(ra);
+
+	dp = (Dirsync_Private *) agmt_get_priv(ra);
+	PR_ASSERT (dp);
+	dp->range_attrs = attrs;
+
+	LDAPDebug0Args( LDAP_DEBUG_TRACE, "<= windows_private_set_move_action\n" );
+}
+
 static PRCallOnceType winsync_callOnce = {0,0};
 
 struct winsync_plugin {

ldap/servers/plugins/replication/windowsrepl.h

@@ -89,6 +89,11 @@ void windows_private_set_one_way(const Repl_Agmt *ra, PRBool value);
 int windows_private_get_move_action(const Repl_Agmt *ra);
 void windows_private_set_move_action(const Repl_Agmt *ra, int value);
 
+Slapi_Entry *windows_private_get_curr_entry(const Repl_Agmt *ra);
+void windows_private_set_curr_entry(const Repl_Agmt *ra, Slapi_Entry *e);
+char **windows_private_get_range_attrs(const Repl_Agmt *ra);
+void windows_private_set_range_attrs(const Repl_Agmt *ra, char **attrs);
+
 /* in windows_connection.c */
 ConnResult windows_conn_connect(Repl_Connection *conn);
 void windows_conn_disconnect(Repl_Connection *conn);

ldap/servers/slapd/entry.c

@@ -2678,8 +2678,8 @@ slapi_entry_attr_get_charray_ext( const Slapi_Entry* e, const char *type, int *n
 {
     char **parray = NULL;
     Slapi_Attr* attr = NULL;
-    slapi_entry_attr_find(e, type, &attr);
     int count = 0;
+    slapi_entry_attr_find(e, type, &attr);
 
     if(numVals == NULL){
         return NULL;
@@ -2711,29 +2711,28 @@ char *
 slapi_entry_attr_get_charptr( const Slapi_Entry* e, const char *type)
 {
     char *p= NULL;
-    Slapi_Attr* attr;
-	slapi_entry_attr_find(e, type, &attr);
-	if(attr!=NULL)
-	{
-		Slapi_Value *v;
+    Slapi_Attr* attr = NULL;
+    slapi_entry_attr_find(e, type, &attr);
+    if(attr!=NULL)
+    {
+        Slapi_Value *v;
                 const struct berval *bvp;
-		slapi_valueset_first_value( &attr->a_present_values, &v);
+        slapi_valueset_first_value( &attr->a_present_values, &v);
                 bvp = slapi_value_get_berval(v);
         p= slapi_ch_malloc(bvp->bv_len + 1);
         memcpy(p, bvp->bv_val, bvp->bv_len);
         p[bvp->bv_len]= '\0';
-	}
+    }
     return p;
 }
 
+/* returned value: attribute value as an integer type */
 int
 slapi_entry_attr_get_int( const Slapi_Entry* e, const char *type)
 {
-    int r= 0;
-    Slapi_Attr* attr;
-	slapi_entry_attr_find(e, type, &attr);
-    if (attr!=NULL)
-    {
+    int r = 0;
+    Slapi_Attr* attr = NULL;
+    if ((0 == slapi_entry_attr_find(e, type, &attr)) && attr) {
 		Slapi_Value *v;
 		slapi_valueset_first_value( &attr->a_present_values, &v);
 		r= slapi_value_get_int(v);
@@ -2741,14 +2740,13 @@ slapi_entry_attr_get_int( const Slapi_Entry* e, const char *type)
     return r;
 }
 
+/* returned value: attribute value as an unsigned integer type */
 unsigned int
 slapi_entry_attr_get_uint( const Slapi_Entry* e, const char *type)
 {
-    unsigned int r= 0;
-    Slapi_Attr* attr;
-	slapi_entry_attr_find(e, type, &attr);
-    if (attr!=NULL)
-    {
+    unsigned int r = 0;
+    Slapi_Attr* attr = NULL;
+    if ((0 == slapi_entry_attr_find(e, type, &attr)) && attr) {
 		Slapi_Value *v;
 		slapi_valueset_first_value( &attr->a_present_values, &v);
 		r= slapi_value_get_uint(v);
@@ -2756,14 +2754,13 @@ slapi_entry_attr_get_uint( const Slapi_Entry* e, const char *type)
     return r;
 }
 
+/* returned value: attribute value as a long integer type */
 long
 slapi_entry_attr_get_long( const Slapi_Entry* e, const char *type)
 {
     long r = 0;
-    Slapi_Attr* attr;
-	slapi_entry_attr_find(e, type, &attr);
-    if (attr!=NULL)
-    {
+    Slapi_Attr* attr = NULL;
+    if ((0 == slapi_entry_attr_find(e, type, &attr)) && attr) {
 		Slapi_Value *v;
 		slapi_valueset_first_value( &attr->a_present_values, &v);
 		r = slapi_value_get_long(v);
@@ -2771,14 +2768,13 @@ slapi_entry_attr_get_long( const Slapi_Entry* e, const char *type)
     return r;
 }
 
+/* returned value: attribute value as an unsigned long integer type */
 unsigned long
 slapi_entry_attr_get_ulong( const Slapi_Entry* e, const char *type)
 {
     unsigned long r = 0;
-    Slapi_Attr* attr;
-	slapi_entry_attr_find(e, type, &attr);
-    if (attr!=NULL)
-    {
+    Slapi_Attr* attr = NULL;
+    if ((0 == slapi_entry_attr_find(e, type, &attr)) && attr) {
 		Slapi_Value *v;
 		slapi_valueset_first_value( &attr->a_present_values, &v);
 		r = slapi_value_get_ulong(v);
@@ -2786,14 +2782,13 @@ slapi_entry_attr_get_ulong( const Slapi_Entry* e, const char *type)
     return r;
 }
 
+/* returned value: attribute value as a long long integer type */
 long long
 slapi_entry_attr_get_longlong( const Slapi_Entry* e, const char *type)
 {
     long long r = 0;
-    Slapi_Attr* attr;
-    slapi_entry_attr_find(e, type, &attr);
-    if (attr!=NULL)
-    {
+    Slapi_Attr* attr = NULL;
+    if ((0 == slapi_entry_attr_find(e, type, &attr)) && attr) {
         Slapi_Value *v;
         slapi_valueset_first_value( &attr->a_present_values, &v);
         r = slapi_value_get_longlong(v);
@@ -2801,14 +2796,13 @@ slapi_entry_attr_get_longlong( const Slapi_Entry* e, const char *type)
     return r;
 }
 
+/* returned value: attribute value as an unsigned long long integer type */
 unsigned long long
 slapi_entry_attr_get_ulonglong( const Slapi_Entry* e, const char *type)
 {
     unsigned long long r = 0;
-    Slapi_Attr* attr;
-    slapi_entry_attr_find(e, type, &attr);
-    if (attr!=NULL)
-    {
+    Slapi_Attr* attr = NULL;
+    if ((0 == slapi_entry_attr_find(e, type, &attr)) && attr) {
         Slapi_Value *v;
         slapi_valueset_first_value( &attr->a_present_values, &v);
         r = slapi_value_get_ulonglong(v);
@@ -2816,14 +2810,13 @@ slapi_entry_attr_get_ulonglong( const Slapi_Entry* e, const char *type)
     return r;
 }
 
+/* returned value: attribute value as a boolean type */
 PRBool
 slapi_entry_attr_get_bool( const Slapi_Entry* e, const char *type)
 {
     PRBool r = PR_FALSE; /* default if no attr */
-    Slapi_Attr* attr;
-    slapi_entry_attr_find(e, type, &attr);
-    if (attr!=NULL)
-    {
+    Slapi_Attr* attr = NULL;
+    if ((0 == slapi_entry_attr_find(e, type, &attr)) && attr) {
         Slapi_Value *v;
         const struct berval *bvp;