Howard Chu wrote:

Johan wrote:

...

Hello all,

...

We have an OpenLDAP instance proxying an active directory with back_meta and> mr_passthru. We also have pcache on top, and as it do not support LDAP_MATCHING_RULE_IN_CHAIN, I looked about implementing it.

...

I found that we could retrieve cached results with pcacheQueryID, but I can't get why this is not used in the current code?

It is of course used in the current code, for refreshing and expiring queries.

...

Plesae see attached patch

Please read https://openldap.org/devel/programming.html

--- servers/slapd/overlays/pcache.c.ori 2023-02-04 10:56:07.910235675 +0100 +++ servers/slapd/overlays/pcache.c 2023-02-04 10:57:14.319386931 +0100 @@ -53,6 +53,16 @@ */ #define PCACHE_MONITOR

+// Not cacheable reasons +#define PCACHEMAXQUERIES_REACHED 0x1 +#define PCACHEATTRSONLY 0x2 +#define PCACHENOTEMPLATE 0x4 +#define PCACHENOATTRSET 0x8

These values should be aligned. OpenLDAP source uses 4-column hard tabs for indents.

+// https://ldapwiki.com/wiki/LDAP_MATCHING_RULE_IN_CHAIN

This URL is currently unreachable. Probably should link to MSDN instead.

+#define LDAP_MATCHING_RULE_IN_CHAIN "1.2.840.113556.1.4.1941"

/* query cache structs */ /* query */

@@ -512,7 +522,7 @@ int attr_cnt; int t_cnt = 0; struct berval bv;

• char *p1, *p2;

• char *p1, *p2, *p3; AttributeDescription *ad; AttributeName *attrs;

@@ -532,9 +542,18 @@ p2 = strchr( p1, '=' ); if ( !p2 ) break;

• if ( p2[-1] == '<' || p2[-1] == '>' ) p2--;
  

• bv.bv_val = p1;
  

• bv.bv_len = p2 - p1;
  

• // Incase of extended operation
  

• p3 = strchr( p1, ':' );
  

• if ( p3 && p3 < p2 ) {
  

• 	// FIXME: Is this valid syntax : "member:1.2.840.113556.1.4.1941:>=3" ?
  

Read RFC4515.

• 	if ( p3[-1] == '<' || p3[-1] == '>' ) p3--;
  

• 	bv.bv_val = p1;
  

• 	bv.bv_len = p3 - p1;
  

• } else {
  

• 	if ( p2[-1] == '<' || p2[-1] == '>' ) p2--;
  

• 	bv.bv_val = p1;
  

• 	bv.bv_len = p2 - p1;
  

• }
  

  ad = NULL; i = slap_bv2ad( &bv, &ad, text ); if ( i ) {

@@ -1136,7 +1155,6 @@ return ret; }

static struct berval* merge_init_final(Operation *op, struct berval* init, struct berval* any, struct berval* final) @@ -1342,8 +1360,13 @@ case LDAP_FILTER_LE: mrule = fs->f_ava->aa_desc->ad_type->sat_ordering; break;

• 	case LDAP_FILTER_EXT:
  

• 		if (0 == strncmp(fi->f_mr_rule_text.bv_val, LDAP_MATCHING_RULE_IN_CHAIN, fi->f_mr_rule_text.bv_len)) {
  

Probably should define the OID in a berval and use ber_bvcmp. We don't use "if (0 == xx)" we use "if (!xx)".

• 			mrule = fs->f_ava->aa_desc->ad_type->sat_equality;
  

• 		}
  

• 		break;
  default:
  

• 		mrule = NULL; 
  

• 		mrule = NULL;
  }
  if (mrule) {
  	const char *text;
  

@@ -1394,6 +1417,7 @@ fi=fi->f_next; break; case LDAP_FILTER_EQUALITY:

• 	case LDAP_FILTER_EXT:
  	if (ret == 0)
  		res = 1;
  	fs=fs->f_next;
  

@@ -1941,6 +1965,19 @@ fstr->bv_len += len; break;

• case LDAP_FILTER_EXT:

• // Concatenate attribute name
  

• if ( f->f_mr_desc ) {
  

• 	ad = f->f_mr_desc;
  

• 	len = STRLENOF( "(::=)" ) + ad->ad_cname.bv_len + f->f_mr_rule_text.bv_len;
  

• 	ret = snprintf( fstr->bv_val+fstr->bv_len, len + 1, "(%s:%s:=)", ad->ad_cname.bv_val, f->f_mr_rule_text.bv_val );
  

• 	assert( ret == len );
  

• 	fstr->bv_len += len;
  

• } else {
  

• 	return -1;
  

• }
  

Again, read RFC4515.

• break;
  

• case LDAP_FILTER_AND: case LDAP_FILTER_OR: case LDAP_FILTER_NOT: {

@@ -2955,17 +2992,22 @@ { slap_overinst *on = (slap_overinst *)op->o_bd->bd_info; cache_manager *cm = on->on_bi.bi_private;

• query_manager* qm = cm->qm;

• query_manager* qm = cm->qm;

Don't make gratuitous whitespace changes.

int i = -1;

Query query; QueryTemplate *qtemp = NULL;

• bindinfo *pbi = NULL;

• bindinfo *pbi = NULL;

• int attr_set = -1;
• CachedQuery *answerable = NULL;
• int cacheable = 0;

• int attr_set = -1;

• CachedQuery *answerable = NULL;

• int cacheable = 0;

• char *fstr;

• Filter *f;

• int ncachereason = 0;

  struct berval tempstr;

@@ -3051,6 +3093,10 @@ if (answerable) break; }

• 	if (cacheable == 0)
  

• 		ncachereason |= PCACHENOTEMPLATE;
  

• } else {
  

• 	ncachereason |= PCACHENOATTRSET;
  

  } op->o_tmpfree( tempstr.bv_val, op->o_tmpmemctx ); }

@@ -3098,7 +3144,16 @@ } } }

• 	/* Replace original query with query to cache ID */
  

• 	fstr = op->o_tmpalloc( sizeof("(pcacheQueryID=12345678-abcd-1234-abcd-123456789abc)")+1, op->o_tmpmemctx );
  

• 	sprintf(fstr, "(pcacheQueryID=%s)", answerable->q_uuid.bv_val);
  

• 	Debug( pcache_debug, "Replace search filter with %s\n", fstr, 0, 0 );
  

No. The fact the query is answerable doesn't mean you can replace the user's filter. The user's filter may be more specific than the cached set, and will only return a subset of entries. This code will break such queries.

• 	f = str2filter(fstr);
  

• 	op->oq_search.rs_filter = f;
  i = cm->db.bd_info->bi_op_search( op, rs );
  

• 	op->o_tmpfree( fstr, op->o_tmpmemctx );
  

  } ldap_pvt_thread_rdwr_wunlock(&answerable->rwlock); /* locked by qtemp->qcfunc (query_containment) */

@@ -3112,11 +3167,14 @@ ldap_pvt_thread_mutex_lock(&cm->cache_mutex); if (cm->num_cached_queries >= cm->max_queries) { cacheable = 0;

• ncachereason |= PCACHEMAXQUERIES_REACHED;
  

  } ldap_pvt_thread_mutex_unlock(&cm->cache_mutex);

• if (op->ors_attrsonly)

• if (op->ors_attrsonly) { cacheable = 0;

• ncachereason |= PCACHEATTRSONLY;
  

• }

  if (cacheable) { slap_callback *cb;

@@ -3170,7 +3228,27 @@ }

} else {

• Debug( pcache_debug, "QUERY NOT CACHEABLE\n" );
  

• switch (ncachereason) {
  

You defined the ncachereason as a set of bitflags. You can't use a switch() statement here, it will just hit default if more than 1 bit is set, which defeats the purpose of tracking any reasons.

• case PCACHEMAXQUERIES_REACHED:
  

• 	Debug( pcache_debug, "QUERY NOT CACHEABLE (max_entries reached)\n",
  

• 			0, 0, 0);
  

• 	break;
  

• case PCACHEATTRSONLY:
  

• 	Debug( pcache_debug, "QUERY NOT CACHEABLE (attrs only)\n",
  

• 			0, 0, 0);
  

• 	break;
  

• case PCACHENOTEMPLATE:
  

• 	Debug( pcache_debug, "QUERY NOT CACHEABLE (No template)\n",
  

• 			0, 0, 0);
  

• 	break;
  

• case PCACHENOATTRSET:
  

• 	Debug( pcache_debug, "QUERY NOT CACHEABLE (No Attrset)\n",
  

• 			0, 0, 0);
  

• 	break;
  

• default:
  

• 	Debug( pcache_debug, "QUERY NOT CACHEABLE (unkown reason)\n",
  

• 			0, 0, 0);
  

• }
  

  }

  return SLAP_CB_CONTINUE;