[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: changes to distributed collect.c overlay



On Sat, Sep 6, 2008 at 4:01 AM, Howard Chu <hyc@symas.com> wrote:
> Pretty good. Just a couple points - it's not necessary to strdup c->argv[]
> before processing it. Since this overlay is the final consumer of the argv,
> there's no reason to preserve it. Trash it at will. strtok should probably
> be used instead of strtok_r in your config code since strtok_r is not
> universal and the code is single-threaded already.
>
> Also, strcat/strncat are never to be used, use lutil_strcopy/strncopy
> instead.
>
> ci_ad_len should probably be ci_ad_num; "_len" is always used for the length
> of strings, "_num" to count anything else.

Have made suggested changes, thanks :)

New diff is attached...

Cheers
Brett
Index: configure.in
===================================================================
RCS file: /repo/OpenLDAP/pkg/ldap/configure.in,v
retrieving revision 1.666
diff -u -r1.666 configure.in
--- configure.in	30 Aug 2008 22:18:08 -0000	1.666
+++ configure.in	6 Sep 2008 06:07:12 -0000
@@ -340,6 +340,7 @@
 	memberof \
 	ppolicy \
 	proxycache \
+	collect \
 	refint \
 	retcode \
 	rwm \
@@ -372,6 +373,8 @@
 	no, [no yes mod], ol_enable_overlays)
 OL_ARG_ENABLE(proxycache,[    --enable-proxycache	  Proxy Cache overlay],
 	no, [no yes mod], ol_enable_overlays)
+OL_ARG_ENABLE(collect,[    --enable-collect      Collect overlay],
+	no, [no yes mod], ol_enable_overlays)
 OL_ARG_ENABLE(refint,[    --enable-refint	  Referential Integrity overlay],
 	no, [no yes mod], ol_enable_overlays)
 OL_ARG_ENABLE(retcode,[    --enable-retcode	  Return Code testing overlay],
@@ -2842,6 +2845,18 @@
 	AC_DEFINE_UNQUOTED(SLAPD_OVER_PROXYCACHE,$MFLAG,[define for Proxy Cache overlay])
 fi
 
+if test "$ol_enable_collect" != no ; then
+        BUILD_COLLECT=$ol_enable_collect
+        if test "$ol_enable_collect" = mod ; then
+                MFLAG=SLAPD_MOD_DYNAMIC
+                SLAPD_DYNAMIC_OVERLAYS="$SLAPD_DYNAMIC_OVERLAYS collect.la"
+        else
+                MFLAG=SLAPD_MOD_STATIC
+                SLAPD_STATIC_OVERLAYS="$SLAPD_STATIC_OVERLAYS collect.o"
+        fi
+        AC_DEFINE_UNQUOTED(SLAPD_OVER_COLLECT,$MFLAG,[define for Collect overlay])
+fi
+
 if test "$ol_enable_refint" != no ; then
 	BUILD_REFINT=$ol_enable_refint
 	if test "$ol_enable_refint" = mod ; then
@@ -3004,6 +3019,7 @@
   AC_SUBST(BUILD_MEMBEROF)
   AC_SUBST(BUILD_PPOLICY)
   AC_SUBST(BUILD_PROXYCACHE)
+  AC_SUBST(BUILD_COLLECT)
   AC_SUBST(BUILD_REFINT)
   AC_SUBST(BUILD_RETCODE)
   AC_SUBST(BUILD_RWM)
Index: include/portable.hin
===================================================================
RCS file: /repo/OpenLDAP/pkg/ldap/include/portable.hin,v
retrieving revision 1.42
diff -u -r1.42 portable.hin
--- include/portable.hin	26 Aug 2008 19:48:17 -0000	1.42
+++ include/portable.hin	6 Sep 2008 06:07:12 -0000
@@ -978,6 +978,9 @@
 /* define for Proxy Cache overlay */
 #undef SLAPD_OVER_PROXYCACHE
 
+/* define for Collect overlay */
+#undef SLAPD_OVER_COLLECT
+
 /* define for Referential Integrity overlay */
 #undef SLAPD_OVER_REFINT
 
Index: servers/slapd/overlays/Makefile.in
===================================================================
RCS file: /repo/OpenLDAP/pkg/ldap/servers/slapd/overlays/Makefile.in,v
retrieving revision 1.49
diff -u -r1.49 Makefile.in
--- servers/slapd/overlays/Makefile.in	7 Jan 2008 23:20:14 -0000	1.49
+++ servers/slapd/overlays/Makefile.in	6 Sep 2008 06:07:13 -0000
@@ -22,6 +22,7 @@
 	dynlist.c \
 	memberof.c \
 	pcache.c \
+	collect.c \
 	ppolicy.c \
 	refint.c \
 	retcode.c \
@@ -83,6 +84,9 @@
 pcache.la : pcache.lo
 	$(LTLINK_MOD) -module -o $@ pcache.lo version.lo $(LINK_LIBS)
 
+collect.la : collect.lo
+	$(LTLINK_MOD) -module -o $@ collect.lo version.lo $(LINK_LIBS)
+
 ppolicy.la : ppolicy.lo
 	$(LTLINK_MOD) -module -o $@ ppolicy.lo version.lo $(LINK_LIBS) $(MODULES_LIBS)
 
Index: servers/slapd/overlays/collect.c
===================================================================
RCS file: /repo/OpenLDAP/pkg/ldap/servers/slapd/overlays/collect.c,v
retrieving revision 1.9
diff -u -r1.9 collect.c
--- servers/slapd/overlays/collect.c	7 Jan 2008 23:20:14 -0000	1.9
+++ servers/slapd/overlays/collect.c	6 Sep 2008 06:07:13 -0000
@@ -43,14 +43,15 @@
 typedef struct collect_info {
 	struct collect_info *ci_next;
 	struct berval ci_dn;
-	AttributeDescription *ci_ad;
+	int ci_ad_num;
+	AttributeDescription *ci_ad[];
 } collect_info;
 
 static int
 collect_cf( ConfigArgs *c )
 {
 	slap_overinst *on = (slap_overinst *)c->bi;
-	int rc = 1;
+	int rc = 1, idx;
 
 	switch( c->op ) {
 	case SLAP_CONFIG_EMIT:
@@ -60,11 +61,30 @@
 			struct berval bv;
 			int len;
 
-			bv.bv_len = ci->ci_dn.bv_len + STRLENOF("\"\" ") +
-				ci->ci_ad->ad_cname.bv_len;
+			// calculate the length & malloc memory
+			bv.bv_len = ci->ci_dn.bv_len + STRLENOF("\"\" ");
+			for (idx=0; idx<ci->ci_ad_num; idx++) {
+				bv.bv_len += ci->ci_ad[idx]->ad_cname.bv_len;
+				if (idx<(ci->ci_ad_num-1)) { 
+					bv.bv_len++;
+				}
+			}
 			bv.bv_val = ch_malloc( bv.bv_len + 1 );
-			len = snprintf( bv.bv_val, bv.bv_len + 1, "\"%s\" %s",
-				ci->ci_dn.bv_val, ci->ci_ad->ad_cname.bv_val );
+
+			// copy the value and update len
+			len = snprintf( bv.bv_val, bv.bv_len + 1, "\"%s\" ", 
+				ci->ci_dn.bv_val);
+                        for (idx=0; idx<ci->ci_ad_num; idx++) {
+				lutil_strcopy( bv.bv_val + strlen(bv.bv_val),
+					ci->ci_ad[idx]->ad_cname.bv_val,
+					bv.bv_len);
+				len += ci->ci_ad[idx]->ad_cname.bv_len;
+				if (idx<(ci->ci_ad_num-1)) {
+					bv.bv_val[len++]=',';
+					bv.bv_val[len]='\0';
+				}
+			}
+
 			assert( len == bv.bv_len );
 			ber_bvarray_add( &c->rvalue_vals, &bv );
 			rc = 0;
@@ -98,8 +118,21 @@
 		collect_info *ci;
 		struct berval bv, dn;
 		const char *text;
-		AttributeDescription *ad = NULL;
+		int idx, count=0;
+		char *arg;
 
+                /* count delimiters in attribute argument */
+                arg = strtok(c->argv[2], ",");
+                while (arg!=NULL) {
+                        count++;
+                        arg = strtok(NULL, ",");
+                }
+
+                /* allocate config info with room for attribute array */
+                ci = ch_malloc( sizeof( collect_info ) +
+                        ( sizeof (AttributeDescription *) * (count + 1)));
+
+		/* validate and normalize dn */
 		ber_str2bv( c->argv[1], 0, 0, &bv );
 		if ( dnNormalize( 0, NULL, NULL, &bv, &dn, NULL ) ) {
 			snprintf( c->cr_msg, sizeof( c->cr_msg ), "%s invalid DN: \"%s\"",
@@ -108,20 +141,34 @@
 				"%s: %s\n", c->log, c->cr_msg, 0 );
 			return ARG_BAD_CONF;
 		}
-		if ( slap_str2ad( c->argv[2], &ad, &text ) ) {
-			snprintf( c->cr_msg, sizeof( c->cr_msg ), "%s attribute description unknown: \"%s\"",
-				c->argv[0], c->argv[2] );
-			Debug( LDAP_DEBUG_CONFIG|LDAP_DEBUG_NONE,
-				"%s: %s\n", c->log, c->cr_msg, 0 );
-			return ARG_BAD_CONF;
+
+		/* load attribute description for attribute list */
+		arg = c->argv[2];
+		for( idx=0; idx<count; idx++) {
+			ci->ci_ad[idx] = NULL;
+
+			if ( slap_str2ad( arg, &ci->ci_ad[idx], &text ) ) {
+				snprintf( c->cr_msg, sizeof( c->cr_msg ), 
+					"%s attribute description unknown: \"%s\"",
+					c->argv[0], arg);
+				Debug( LDAP_DEBUG_CONFIG|LDAP_DEBUG_NONE,
+					"%s: %s\n", c->log, c->cr_msg, 0 );
+				return ARG_BAD_CONF;
+			}
+			while(*arg!='\0') {
+				arg++; // skip to end of argument
+			}
+			if (idx<count-1) {
+				arg++; // skip inner delimiters
+			}
 		}
 
 		/* The on->on_bi.bi_private pointer can be used for
 		 * anything this instance of the overlay needs.
 		 */
-		ci = ch_malloc( sizeof( collect_info ));
-		ci->ci_ad = ad;
-		ci->ci_dn = dn;
+                ci->ci_ad[count] = NULL;
+                ci->ci_ad_num = count;
+                ci->ci_dn = dn;
 		ci->ci_next = on->on_bi.bi_private;
 		on->on_bi.bi_private = ci;
 		rc = 0;
@@ -167,6 +214,50 @@
 }
 
 static int
+collect_modify( Operation *op, SlapReply *rs)
+{
+        slap_overinst *on = (slap_overinst *) op->o_bd->bd_info;
+        collect_info *ci = on->on_bi.bi_private;
+	Modifications *ml;
+        char errMsg[100];
+	int rc, idx;
+
+        for ( ml = op->orm_modlist; ml != NULL; ml = ml->sml_next) {
+                for (; ci; ci=ci->ci_next ) {
+
+                        /* Is this entry an ancestor of this collectinfo ? */
+                        if (!dnIsSuffix(&op->o_req_ndn, &ci->ci_dn)) {
+                                /* this collectinfo does not match */
+                                continue;
+                        }
+
+                        /* Is this entry the same as the template DN ? */
+                        if ( strcmp(op->o_req_ndn.bv_val, ci->ci_dn.bv_val) == 0) {
+                                /* dont apply change collect-ed attributes */
+                                continue;
+                        }
+
+			/* check for collect attributes - disallow modify if present */
+                        for(idx=0; idx<ci->ci_ad_num; idx++) {
+				if (strcmp(ml->sml_desc->ad_cname.bv_val,
+						ci->ci_ad[idx]->ad_cname.bv_val) == 0) {
+					rs->sr_err = LDAP_UNWILLING_TO_PERFORM;
+					snprintf( errMsg, sizeof( errMsg ), 
+                        			"cannot change virtual attribute '%s'",
+						ci->ci_ad[idx]->ad_cname.bv_val);
+					rs->sr_text = errMsg;
+        				send_ldap_result( op, rs );
+        				return rs->sr_err;
+				}
+			}
+		}
+
+	}
+
+	return SLAP_CB_CONTINUE;
+}
+
+static int
 collect_response( Operation *op, SlapReply *rs )
 {
 	slap_overinst *on = (slap_overinst *) op->o_bd->bd_info;
@@ -181,35 +272,51 @@
 		op->o_bd->bd_info = (BackendInfo *)on->on_info;
 
 		for (; ci; ci=ci->ci_next ) {
-			BerVarray vals = NULL;
+			int idx=0;
+
+			/* Is this entry an ancestor of this collectinfo ? */
+			if (!dnIsSuffix(&rs->sr_entry->e_nname, &ci->ci_dn)) {
+			        /* collectinfo does not match */
+			        continue;
+			}
 
-			/* Is our configured entry an ancestor of this one? */
-			if ( !dnIsSuffix( &rs->sr_entry->e_nname, &ci->ci_dn ))
+			/* Is this entry the same as the template DN ? */
+			if ( strcmp(rs->sr_entry->e_nname.bv_val, ci->ci_dn.bv_val) == 0) {
+				/* dont apply change to parent */
 				continue;
+			}
 
-			/* Extract the values of the desired attribute from
-			 * the ancestor entry
-			 */
-			rc = backend_attribute( op, NULL, &ci->ci_dn, ci->ci_ad, &vals, ACL_READ );
-
-			/* If there are any values, merge them into the
-			 * current entry
-			 */
-			if ( vals ) {
-				/* The current entry may live in a cache, so
-				 * don't modify it directly. Make a copy and
-				 * work with that instead.
-				 */
-				if ( !( rs->sr_flags & REP_ENTRY_MODIFIABLE )) {
-					rs->sr_entry = entry_dup( rs->sr_entry );
-					rs->sr_flags |= REP_ENTRY_MODIFIABLE |
-						REP_ENTRY_MUSTBEFREED;
+                        /* The current entry may live in a cache, so
+                        * don't modify it directly. Make a copy and
+                        * work with that instead.
+                        */
+                        if ( !( rs->sr_flags & REP_ENTRY_MODIFIABLE )) {
+                                rs->sr_entry = entry_dup( rs->sr_entry );
+                                rs->sr_flags |= REP_ENTRY_MODIFIABLE |
+                                        REP_ENTRY_MUSTBEFREED;
+                        }
+
+			/* Loop for each attribute in this collectinfo */
+			for(idx=0; idx<ci->ci_ad_num; idx++) {
+                        	BerVarray vals = NULL;
+
+				/* Extract the values of the desired attribute from
+			 	 * the ancestor entry */
+				rc = backend_attribute( op, NULL, &ci->ci_dn, 
+					ci->ci_ad[idx], &vals, ACL_READ );
+
+				/* If there are any values, merge them into the
+			 	 * current search result
+			 	 */
+				if ( vals ) {
+					attr_merge( rs->sr_entry, ci->ci_ad[idx], 
+						vals, NULL );
+					ber_bvarray_free_x( vals, op->o_tmpmemctx );
 				}
-				attr_merge( rs->sr_entry, ci->ci_ad, vals, NULL );
-				ber_bvarray_free_x( vals, op->o_tmpmemctx );
 			}
 		}
-	}
+        }
+
 	/* Default is to just fall through to the normal processing */
 	return SLAP_CB_CONTINUE;
 }
@@ -221,6 +328,7 @@
 
 	collect.on_bi.bi_type = "collect";
 	collect.on_bi.bi_db_destroy = collect_destroy;
+        collect.on_bi.bi_op_modify = collect_modify;
 	collect.on_response = collect_response;
 
 	collect.on_bi.bi_cf_ocs = collectocs;