#48363 Support for rfc3673 '+' to return operational attributes
Closed: Fixed None Opened 4 years ago by firstyear.

We should implement OID 1.3.6.1.4.1.4203.1.5.1 as a supportedFeature in 389-ds as per rfc3673.

https://tools.ietf.org/html/rfc3673


Note: Check GER as this implements the extension already.

Hi William, I found a couple of minor issues... 1) I think the year of the copyright would be 2016? ;) * Copyright (C) 2005 Red Hat, Inc. 2) You defined OID in slapi-plugin.h: 5622 #define LDAP_FEATURE_RFC3673_ALLOPERATIONAL "1.3.6.1.4.1.4203.1.5.1" The OID is defined in /usr/include/ldap.h. #define LDAP_FEATURE_ALL_OP_ATTRS "1.3.6.1.4.1.4203.1.5.1" /* RFC 3673 */ So, I'd recommend to put your define in #if !defined(USE_OPENLDAP) and use the same macro LDAP_FEATURE_ALL_OP_ATTRS for not USE_OPENLDAP case (== MOZLDAP). 3) For some reason, features.c does not include slapi-plugin.h and the server fails to start for me as follows... Isn't it needed? {{{ Program received signal SIGILL, Illegal instruction. __GI___pthread_rwlock_unlock (rwlock=0x5555557d67a0) at pthread_rwlock_unlock.c:34 34 if (ELIDE_UNLOCK (rwlock->__data.__writer == 0 #0 __GI___pthread_rwlock_unlock (rwlock=0x5555557d67a0) at pthread_rwlock_unlock.c:34 #1 0x00007ffff7b64cfb in slapi_rwlock_unlock (rwlock=0x5555557d67a0) at ldap/servers/slapd/slapi2nspr.c:254 #2 0x00007ffff7aef9af in slapi_get_supported_features_copy (ftroidsp=0x7fffffff7120) at ldap/servers/slapd/features.c:46 #3 0x000055555558660f in read_root_dse (pb=0x5555559db910, e=0x555555aab500, entryAfter=0x0, returncode=0x7fffffff72a4, returntext=0x7fffffff72e0 "", arg=0x0) at ldap/servers/slapd/rootdse.c:196 #4 0x00007ffff7ae1691 in dse_call_callback (pdse=0x555555927a00, pb=0x5555559db910, operation=4, flags=1, entryBefore=0x555555aab500, entryAfter=0x0, returncode=0x7fffffff72a4, returntext=0x7fffffff72e0 "") at ldap/servers/slapd/dse.c:2634 #5 0x00007ffff7adf256 in dse_search (pb=0x5555559db910) at ldap/servers/slapd/dse.c:1737 #6 0x00007ffff7b27e91 in op_shared_search (pb=0x5555559db910, send_result=1) at ldap/servers/slapd/opshared.c:802 #7 0x00007ffff7b3deb4 in search_internal_callback_pb (pb=0x5555559db910, callback_data=0x7fffffffda10, prc=0x7ffff7b3d939 <internal_plugin_result_callback>, psec=0x7ffff7b3d84c <internal_plugin_search_entry_callback>, prec=0x7ffff7b3d8cc <internal_plugin_search_referral_callback>) at ldap/servers/slapd/plugin_internal_op.c:783 #8 0x00007ffff7b3da0b in search_internal_pb (pb=0x5555559db910) at ldap/servers/slapd/plugin_internal_op.c:636 #9 0x00007ffff7b3d656 in slapi_search_internal (base=0x7fffea672a95 "", scope=0, filter=0x7fffea672a85 "(objectclass=*)", controls=0x0, attrs=0x7fffffffdaf0, attrsonly=0) at ldap/servers/slapd/plugin_internal_op.c:480 #10 0x00007fffea671693 in views_cache_build_view_list (pViews=0x7fffea874040 <thecache>) at ldap/servers/plugins/views/views.c:1154 #11 0x00007fffea670941 in views_cache_create () at ldap/servers/plugins/views/views.c:448 #12 0x00007fffea670304 in views_start (pb=0x555555ac09f8) at ldap/servers/plugins/views/views.c:217 #13 0x00007ffff7b36ffe in plugin_call_func (list=0x5555559d20f0, operation=212, pb=0x555555ac09f8, call_one=1) at ldap/servers/slapd/plugin.c:1987 #14 0x00007ffff7b36e7d in plugin_call_one (list=0x5555559d20f0, operation=212, pb=0x555555ac09f8) at ldap/servers/slapd/plugin.c:1937 #15 0x00007ffff7b3673f in plugin_dependency_startall (argc=7, argv=0x7fffffffe438, errmsg=0x7ffff7b9ce9c "plugin startup failed\n", operation=212, plugin_list=0x0) at ldap/servers/slapd/plugin.c:1745 #16 0x00007ffff7b36d94 in plugin_startall (argc=7, argv=0x7fffffffe438, plugin_list=0x0) at ldap/servers/slapd/plugin.c:1898 #17 0x000055555557d686 in main (argc=7, argv=0x7fffffffe438) at ldap/servers/slapd/main.c:1059 }}}

Thanks for the review Noriko.

I didn't have that issue with building, but you did correctly notice I was missing that header. I have added it now. I've also fixed up the other points you raised. Here is a diff of what I changed, and the rebased patch is attached to the thread.

{{{

diff --git a/ldap/servers/slapd/features.c b/ldap/servers/slapd/features.c
index aade6c3..077002e 100644
--- a/ldap/servers/slapd/features.c
+++ b/ldap/servers/slapd/features.c
@@ -1,6 +1,5 @@
/* BEGIN COPYRIGHT BLOCK
- * Copyright (C) 2001 Sun Microsystems, Inc. Used by permission.
- * Copyright (C) 2005 Red Hat, Inc.
+ * Copyright (C) 2016 Red Hat, Inc.
* All rights reserved.

* License: GPL (version 3 or any later version).
@@ -15,6 +14,7 @@

#include <stdio.h>
#include "slap.h"
+#include "slapi-plugin.h"

static char *supported_features = NULL;
static Slapi_RWLock
supported_features_lock = NULL;
@@ -28,7 +28,7 @@ init_features( void )
"init_features: failed to create lock.\n");
exit(1);
}
- slapi_register_supported_feature( LDAP_FEATURE_RFC3673_ALLOPERATIONAL );
+ slapi_register_supported_feature( LDAP_FEATURE_ALL_OP_ATTRS );
}

int
diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h
index 5361973..f66ccd0 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -5619,7 +5619,9 @@ char *slapi_get_supported_extended_ops_copy( void );
* routines for dealing with supported features
/

-#define LDAP_FEATURE_RFC3673_ALLOPERATIONAL "1.3.6.1.4.1.4203.1.5.1"
+#ifndef USE_OPENLDAP
+#define LDAP_FEATURE_ALL_OP_ATTRS "1.3.6.1.4.1.4203.1.5.1"
+#endif

int slapi_get_supported_features_copy( char ***ftroidsp );

}}}

Looks good.

It's a very minor thing, but this is more straightforward?
{{{
+#ifndef LDAP_FEATURE_ALL_OP_ATTRS
+#define LDAP_FEATURE_ALL_OP_ATTRS "1.3.6.1.4.1.4203.1.5.1"
+#endif
}}}
For instance, this header "slapi-plugin.h" has lots of similar macros...
{{{
/
* filter types
* openldap defines these, but not mozldap
/

ifndef LDAP_FILTER_AND

define LDAP_FILTER_AND 0xa0L

endif

}}}

commit f4b97595525e07ca22b1e361edabf207d0dbba48

To ssh://git.fedorahosted.org/git/389/ds.git
1d4fcea..08c40e8 master -> master

Thanks!

git patch file (master) -- one line fix; symptom: setup ds fails with server's crash.
0001-Ticket-48363-Support-for-rfc3673-to-return-operation.3.patch

One line fix; pushed to master:
4e39b5b..0cf249e master -> master
commit 0cf249e

Yep, the test passes for me.

I think that it should be named something like "dirsrvtests/tests/suites/filter/rfc3673_all_oper_attrs_test.py"

I also liked the comments in my test that referenced the rfc. Maybe you could transplant these?

Otherwise, functionality wise, I'm happy to ack, and if you want to make these other changes, that would be awesome.

William, thank you for the review. :) Applied.

To ssh://git.fedorahosted.org/git/389/ds.git
17f30c6..03c1e86 master -> master
commit 03c1e86
Author: Simon Pichugin spichugi@redhat.com
Date: Fri May 20 10:54:51 2016 +0200

Metadata Update from @firstyear:
- Issue assigned to firstyear
- Issue set to the milestone: 1.3.5 backlog

3 years ago

Login to comment on this ticket.

Metadata