Linked to Bugzilla bug: https://bugzilla.redhat.com/show_bug.cgi?id=768084

I would like to add a request to have a read-only, check method available for this plugin. That way you could produce a report or changes the plugin WOULD make if executed. It would also be nice if you could supply an ldif of theoretical hosts to see where they would land.

To include all the requested features, I've created 3 new tasks for the automember plugin:

[1] Rebuild the membership

This will scan the database for matching entries, and add them to their proper groups

dn: cn=my rebuild task, cn=automember rebuild membership,cn=tasks,cn=config
objectClass: top
objectClass: extensibleObject
cn: my rebuild task
basedn: dc=example,dc=com
filter: (uid=*)
scope: sub

[2] Export the updates

This will create a ldif file with all the changes the server would of done with the rebuild membership task.

dn: cn=my export task, cn=automember export updates,cn=tasks,cn=config
objectClass: top
objectClass: extensibleObject
cn: my export task
basedn: dc=example,dc=com
filter: (uid=*)
scope: sub
ldif: /tmp/automem-updates.ldif

[3] Map the updates of entries provided in a ldif to see what the server would do with them

We take the LDIF entries in the ldif file from "ldif_in", and write the modifications that the server would do to the "ldif_out" file.

dn: cn=my map task, cn=automember map updates,cn=tasks,cn=config
objectClass: top
objectClass: extensibleObject
cn: my map task
ldif_in: /tmp/entries.ldif
ldif_out: /tmp/automem-updates.ldif

Testing looks good so far, if everything goes well I will send it out for review.

The design looks good. I have a request on handling DNs. We are trying to use Slapi_DN instead of char DN. That way, we could avoid the unnecessary dn normalization in case the normalization is needed. Could it be possible to change the type of base_dn and bind_dn to Slapi_DN ?

For instance, we have SLAPI_REQUESTION_SDN (as well as SLAPI_REQUESTOR_DN), which is used to get bind_dn. Not sure about "base_dn", but it is not normalized in the code? Is it guaranteed? If not, you'd better create Slapi_DN from the given base_dn...

1949    typedef struct _task_data 
1950    { 
1951        char *filter_str; 
1952        char *ldif_out; 
1953        char *ldif_in; 
1954        char *base_dn; <===
1955        char *bind_dn; <===
1956        int scope; 
1957    } task_data;

The rest looks good to me.

The bind dn is only used for the local thread storage for the internalModifiersname, and the DN is already normalized.

The base DN is not normalized, so I will work on that.

Thanks,
Mark

Thanks, Mark! One more... Instead of using slapi_search_internal_set_pb, could you call slapi_search_internal_set_pb_ext? It takes Slapi_DN * and it saves an internal dn normalization.

2167 slapi_search_internal_set_pb(search_pb, slapi_sdn_get_dn(td->base_dn), td->scope, td->filter_str, NULL,
2168 0, NULL, NULL, automember_get_plugin_id(), 0);

2367 slapi_search_internal_set_pb(search_pb, slapi_sdn_get_dn(td->base_dn), td->scope, td->filter_str, NULL,
2368 0, NULL, NULL, automember_get_plugin_id(), 0);

void slapi_search_internal_set_pb_ext(Slapi_PBlock pb, Slapi_DN sdn,
int scope, const char filter, char attrs, int attrsonly,
LDAPControl controls, const char uniqueid,

Hi Noriko,

But the DN was never normalized, doing it this way normalizes it. Also this is for a task, so the overhead of the normalization won't impact normal DS operations. What do you think?

Thanks,
Mark

If you don't call slapi_search_internal_set_pb_ext, op_shared_search creates Slapi_DN from slapi_sdn_get_dn(td->base_dn) again. It would normalize it again internally. I thought it's redundant... Yeah, you may not have to worry the task performance too much. Just we are hoping to get rid of "char *DN" eventually...

228 op_shared_search (Slapi_PBlock *pb, int send_result)
[...]
277 slapi_pblock_get(pb, SLAPI_ORIGINAL_TARGET_DN, &base);
278 slapi_pblock_get(pb, SLAPI_SEARCH_TARGET_SDN, &sdn);
279
280 if (NULL == sdn) {
281 sdn = slapi_sdn_new_dn_byval(base);
282 slapi_pblock_set(pb, SLAPI_SEARCH_TARGET_SDN, sdn);
283 }

in automember_rebuild_task_thread() - need to free the entries and the search_pb

why use read_next_entry() and ldif_record_end()? We already use ldif_read_record() or ldif_get_entry() - will those functions not work?

otherwise, looks good

I looked to see how the dse.ldif was read in, that's why I did it the way I did it. I didn't really like that approach anyway, and ldif_read_record/ldif_get_entry looks way easier. Reworking fix, thanks for pointing that out.

Replying to [comment:17 mreynolds]:

I looked to see how the dse.ldif was read in, that's why I did it the way I did it. I didn't really like that approach anyway, and ldif_read_record/ldif_get_entry looks way easier. Reworking fix, thanks for pointing that out.

Ah, ok. We should have another task to replace that code in dse.c

revised patch using slapi_dn for the base dn's
0001-Ticket-20-Allow-automember-to-work-on-entries-that-h.patch

[mareynol@localhost servers]$ git merge ticket20
Updating b8e6b13..a4e4edc
Fast-forward
ldap/servers/plugins/automember/automember.c | 718 +++++++++++++++++++++++++-
1 files changed, 710 insertions(+), 8 deletions(-)

[mareynol@localhost servers]$ git push origin master
Counting objects: 13, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 5.60 KiB, done.
Total 7 (delta 4), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
b8e6b13..a4e4edc master -> master

Added initial screened field value.

What if the error is some other NSPR error other than an OS error? If you want to convert NSPR error codes to strings, you can use the function PR_ErrorToString() and use PR_LANGUAGE_I_DEFAULT as the PRLanguageCode language argument.

Alternately, you could export slapd_pr_strerror() and/or slapd_system_strerror() to be public slapi functions, or just include slapi-private.h in automember and use them directly.

In the automember code, since you are trying to get the error code from PR_Open, which is an NSPR API function, you should call PR_GetError() to get the error code. NSPR is supposed to map OS errors (e.g. errno) to a corresponding NSPR errors (e.g. if PR_Open() calls open() and gets ENOENT, NSPR will map that to PR_FILE_NOT_FOUND_ERROR and PR_GetError() will return PR_FILE_NOT_FOUND_ERROR). You should not have to call PR_GetOSError, you should just use PR_GetError(). If that is not working, if PR_GetError() returns a 0 or a nonsensical error code, then we will have to rely on PR_GetOSError() instead (and file a bug against NSPR). When you have an NSPR error code, you have to call slapd_pr_strerror() to map that error code to a string - you cannot use slapd_pr_strerror with OS errors (i.e. errno), nor can you use slapd_system_strerror with NSPR error codes.

Finally, you could save a lot of code changes by just creating slapi wrappers around the slapd functions e.g. just have slapi_pr_strerror() call slapd_pr_strerror(), and have slapi_system_strerror() call slapd_system_strerror().

Replying to [comment:26 rmeggins]:

In the automember code, since you are trying to get the error code from PR_Open, which is an NSPR API function, you should call PR_GetError() to get the error code. NSPR is supposed to map OS errors (e.g. errno) to a corresponding NSPR errors (e.g. if PR_Open() calls open() and gets ENOENT, NSPR will map that to PR_FILE_NOT_FOUND_ERROR and PR_GetError() will return PR_FILE_NOT_FOUND_ERROR). You should not have to call PR_GetOSError, you should just use PR_GetError(). If that is not working, if PR_GetError() returns a 0 or a nonsensical error code, then we will have to rely on PR_GetOSError() instead (and file a bug against NSPR).

PR_GetError() does return zero, that why I used PR_GetOSError().

When you have an NSPR error code, you have to call slapd_pr_strerror() to map that error code to a string - you cannot use slapd_pr_strerror with OS errors (i.e. errno), nor can you use slapd_system_strerror with NSPR error codes.

Finally, you could save a lot of code changes by just creating slapi wrappers around the slapd functions e.g. just have slapi_pr_strerror() call slapd_pr_strerror(), and have slapi_system_strerror() call slapd_system_strerror().

Ok, I'll undo all the changes and use wrappers.

latest patch still uses slapi_pr_strerror() with a system error code - should use slapi_system_strerror instead

Improved error codes, and made two error code functions available to the plugin API
0001-Ticket-20-Allow-automember-to-work-on-entries-that.patch

I thought PR_GetOSError() returned a PR error code, and that you were referring to the call to strerror(). My bad. New patch attached.

[mareynol@localhost ds]$ git merge ticket20
Updating 4850b27..2b98d67
Fast-forward
ldap/servers/plugins/automember/automember.c | 44 +++++++++++++++----------
ldap/servers/slapd/errormap.c | 10 ++++++
ldap/servers/slapd/slapi-plugin.h | 19 +++++++++++
3 files changed, 55 insertions(+), 18 deletions(-)

[mareynol@localhost ds]$ git push origin master
Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 1.65 KiB, done.
Total 10 (delta 7), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
4850b27..2b98d67 master -> master

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/20

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.