Description of problem: related bug #193799 create a load_schema task using Task mechanism (http://directory.fedoraproject.org/wiki/Slapi_Task_API) Requirements . some locking mechanism is needed while schema is loaded (post-op phase) * oc_lock_read/write: R/W lock used to protect the global objclass linked list (struct objclass) * schema_dse_lock_read/write: R/W lock used to serialize access to the schema DSE Needs to apply both write lock Proposed APIs: . locking void slapi_load_schemafile_lock(); Desccription: Call oc_lock_write and schema_dse_lock_write; void slapi_load_schemafile_unlock(); Desccription: Call both oc_unlock and schema_dse_unlock; , preop: schema validation A pre-op phase that does the validation and a post-op phase that actually applies the changes. In the pre-op validation phase, if something fails, we should abort the task; report the error; and go back to the original schema or leave the original schema without any changes. So we either need to do the validation using a copy of the schema info, or we need to be able to make a copy of them at the beginning of validation and restore them if something goes wrong. [available schema checking functions] schema_check_name ( pnew_oc->oc_name, PR_FALSE, errorbuf, errorbufsize ) /* check to see if the objectclass name is valid */ schema_check_oid ( psbOcName->buffer, psbOcOid->buffer, PR_FALSE, errorbuf, errorbufsize) /* check to see if the oid is valid */ schema_check_oc_attrs ( pnew_oc, errorbuf, errorbufsize, 0 /* don't strip options */ ) /* check to see if the oc's attributes are valid */ note: These functions are called via read_at_ldif via init_schema_dse. Proposed API: Slapi_Entry **slapi_validate_schema_files(const char *configdir); Desccription: Validate schema files found in configdir Modify init_schema_dse to accept a flag to switch storing dse_tree or not, and call it. . postop: load schema files if schema validation is successful What needs to be done for loading is calling dse_add_entry_pb per each schema entry, which is passed from the preop? 634 static int 635 dse_read_one_file(struct dse *pdse, const char *filename, Slapi_PBlock *pb, 636 int primary_file ) 637 { [...] 735 if(dse_call_callback(pdse, pb, DSE_OPERATION_READ, 736 DSE_FLAG_PREOP, e, NULL, &returncode, 737 returntext) == SLAPI_DSE_CALLBACK_OK) 738 { 739 /* this will free the entry if not added, so it is 740 definitely consumed by this call */ 741 *dse_add_entry_pb(pdse, e, pb);* 742 } Proposed API: int slapi_validate_schema_files( Slapi_Entry **schema_entries); Descrption: Load schema entries Store each entry in the dse_tree -- dse_add_entry_pb. Results: If it fails, report the error message and stop the server? The task_load_schemafile_thread would look like this? static void task_load_schemafile_thread(void *arg) { Slapi_PBlock *pb = (Slapi_PBlock *)arg; Slapi_Task *task = pb->pb_task; Slapi_Entry **schema_entries = NULL; char *configdir = NULL; int rv = 0; task->task_work = 1; task->task_progress = 0; task->task_state = SLAPI_TASK_RUNNING; slapi_task_status_changed(task); slapi_task_log_notice(task, "Load schema file task starts (arg: %s) ...\n", pb->pb_seq_val); slapi_pblock_get(pb, SLAPI_GET_CONFIGDIR, &configdir); /* making this up... */ schema_entries = slapi_validate_schema_files(configdir); if (schema_entries) { slapi_load_schemafile_lock(); /* lock schema */ rv = slapi_validate_schema_files(schema_entries); slapi_load_schemafile_unlock(); if (0 != rv) { /* FATAL: dynamic schema file load failed */ } } slapi_task_log_notice(task, "Load schema file task finished."); slapi_task_log_status(task, "Load schema file task finished."); task->task_progress = 1; task->task_exitcode = rv; task->task_state = SLAPI_TASK_FINISHED; slapi_task_status_changed(task); slapi_ch_free((void **)&pb->pb_seq_val); slapi_pblock_destroy(pb); }
Created attachment 307257 [details] cvs diffs Modified files: ldap/servers/slapd/attrsyntax.c ldap/servers/slapd/backend.c ldap/servers/slapd/backend_manager.c ldap/servers/slapd/dse.c ldap/servers/slapd/entry.c ldap/servers/slapd/mapping_tree.c ldap/servers/slapd/pblock.c ldap/servers/slapd/proto-slap.h ldap/servers/slapd/schema.c ldap/servers/slapd/schemaparse.c ldap/servers/slapd/slap.h ldap/servers/slapd/slapi-plugin.h ldap/servers/slapd/slapi-private.h ldap/servers/slapd/back-ldbm/init.c ldap/ldif/template-dse.ldif.in Makefile.am New file: ldap/servers/plugins/schema_reload/schema_reload.c Description: see http://directory.fedoraproject.org/wiki/Dynamically_Reload_Schema In addition to the test cases in the wiki page, ran the concurrency check (test case 6) in the wiki against the valgrind'ed server. Some memory leaks and attribute syntax info (struct asyntaxinfo) leaks were found. The leaks are also fixed.
Created attachment 308278 [details] New file: ldapserver/ldap/servers/plugins/schema_reload/schema_reload.c Thanks to Nathan for his reviews and comments. Revised the new file schema_reload.c following his suggestions. . renamed the functions from "task_" to "schemareload_" to set the clearer module identity. . eliminated useless internal functions.
Created attachment 308379 [details] cvs diffs
Description of Comment #3: One more fix based upon the comments by Nathan (Thanks!!) Reusing SLAPI_TASK_FLAGS in the schema reload task is confusing since the real location the flags being used is in the schema code, not in the task code. Instead, introduced SLAPI_SCHEMA_FLAGS (pb_schema_flags) to pblock. To prevent for the pblock size not to grow, merged SLAPI_SCHEMA_USER_DEFINED_ONLY into SLAPI_SCHEMA_FLAGS. files: ldap/servers/slapd/slap.h ldap/servers/slapd/slapi-plugin.h ldap/servers/slapd/slapi-private.h ldap/servers/slapd/schema.c ldap/servers/slapd/pblock.c ldap/servers/slapd/dse.c
Looks good, but I don't think you want to change SLAPD_VENDOR_NAME and SLAPD_VERSION_STR in slap.h.
Created attachment 308400 [details] cvs diffs (final) (In reply to comment #5) > Looks good, but I don't think you want to change SLAPD_VENDOR_NAME and > SLAPD_VERSION_STR in slap.h. Thanks, Nathan! I double-checked I was not changing these strings... Attached is the final diffs.
Created attachment 308403 [details] cvs commit message Reviewed by Nathan (Thanks a lot!) Checked in into CVS HEAD.
*** Bug 193799 has been marked as a duplicate of this bug. ***
Does the PR Hashtable guarantee that deletion of hash entries from within the hash table enumeration is safe? At https://bugzilla.redhat.com/attachment.cgi?id=308400&action=diff#ldap/servers/slapd/backend.c_sec2 Is be guaranteed to be non-NULL? If not, perhaps you could do return ((be == NULL) || (BE_STATE_DELETED == be->be_state)); assuming if be is NULL that means the be has been deleted. At https://bugzilla.redhat.com/attachment.cgi?id=308400&action=diff#ldap/servers/slapd/slap.h_sec1 In general, new items must be added at the end of structures in order to preserve binary compatibility. However, in this case, since it is unlikely there are other database plugins not built by us, this is safe.
Thanks for your comments, Rich. (In reply to comment #9) > Does the PR Hashtable guarantee that deletion of hash entries from within the > hash table enumeration is safe? I followed the coding style of attr_syntax_delete_all_not_flagged which calls attr_syntax_delete_if_not_flagged in the hash table enumeration. Maybe, there could be more efficient way to do the same task. Do you have any idea? > At > https://bugzilla.redhat.com/attachment.cgi?id=308400&action=diff#ldap/servers/slapd/backend.c_sec2 > > Is be guaranteed to be non-NULL? If not, perhaps you could do > return ((be == NULL) || (BE_STATE_DELETED == be->be_state)); > assuming if be is NULL that means the be has been deleted. Modified it as you suggested. > At > https://bugzilla.redhat.com/attachment.cgi?id=308400&action=diff#ldap/servers/slapd/slap.h_sec1 > > In general, new items must be added at the end of structures in order to > preserve binary compatibility. However, in this case, since it is unlikely > there are other database plugins not built by us, this is safe. Thanks. (I did it, again... :p) Modified it as you suggested.
(In reply to comment #10) > Thanks for your comments, Rich. > > (In reply to comment #9) > > Does the PR Hashtable guarantee that deletion of hash entries from within the > > hash table enumeration is safe? > > I followed the coding style of attr_syntax_delete_all_not_flagged which calls > attr_syntax_delete_if_not_flagged in the hash table enumeration. Maybe, there > could be more efficient way to do the same task. Do you have any idea? Ok. If that's the way the existing code does it, then that's ok. I don't know if the pr hashtable api provides a better way to do this. > > > At > > > https://bugzilla.redhat.com/attachment.cgi?id=308400&action=diff#ldap/servers/slapd/backend.c_sec2 > > > > Is be guaranteed to be non-NULL? If not, perhaps you could do > > return ((be == NULL) || (BE_STATE_DELETED == be->be_state)); > > assuming if be is NULL that means the be has been deleted. > > Modified it as you suggested. > > > At > > > https://bugzilla.redhat.com/attachment.cgi?id=308400&action=diff#ldap/servers/slapd/slap.h_sec1 > > > > In general, new items must be added at the end of structures in order to > > preserve binary compatibility. However, in this case, since it is unlikely > > there are other database plugins not built by us, this is safe. > > Thanks. (I did it, again... :p) Modified it as you suggested.
Created attachment 308554 [details] cvs diff slap.h backend.c Files: ldapserver/ldap/servers/slapd/slap.h /backend.c Description: fixing the code as suggested by Rich in the Comment #9.
Oops, please ignore the diffs of slap.h in comment #12. This is the right one... Index: slap.h =================================================================== RCS file: /cvs/dirsec/ldapserver/ldap/servers/slapd/slap.h,v retrieving revision 1.33 diff -t -w -U2 -r1.33 slap.h --- slap.h 4 Jun 2008 22:22:55 -0000 1.33 +++ slap.h 6 Jun 2008 17:50:29 -0000 @@ -777,6 +777,6 @@ IFP plg_un_db_init_instance; /* initializes new db instance */ IFP plg_un_db_wire_import; /* fast replica update */ - IFP plg_un_db_add_schema; /* add schema */ IFP plg_un_db_verify; /* verify db files */ + IFP plg_un_db_add_schema; /* add schema */ } plg_un_db; #define plg_bind plg_un.plg_un_db.plg_un_db_bind
Created attachment 308555 [details] cvs commit message Thanks to Rich for his reviews and comments. Checked in into CVS HEAD.
Created attachment 308847 [details] cvs diffs Files: ldapserver/ldap/servers/slapd/slapi-private.h /proto-slap.h /schema.c Bug description: broke the build on Fedora 9. First, slapi_load_schemafile_(un)lock was planned to be a public API, but I found it's not necessary. Thus, I made it static in schema.c, but forgot to remove the declaration from proto-slap.h. Since it's not a part of slapi api, I'm removing the "slapi_" from the static function names.
Created attachment 308848 [details] cvs commit message Reviewed by Nathan (Thank you!!) Checked in into CVS HEAD.
Created attachment 311975 [details] cvs diff schema_reload.c Resolves: #436837 Summary: Dynamically reload schema via task interface Description: cleaned up compile warnings. CVS: ---------------------------------------------------------------------- CVS: Modified Files: CVS: schema_reload.c CVS: ---------------------------------------------------------------------- Checking in schema_reload.c; /cvs/dirsec/ldapserver/ldap/servers/plugins/schema_reload/schema_reload.c,v <-- schema_reload.c new revision: 1.3; previous revision: 1.2 done
This again seems more of a task than a bug and is a duplicate of a feature request. Can't really verify a fix. Should this just be closed?
(In reply to comment #18) > This again seems more of a task than a bug and is a duplicate of a feature > request. Can't really verify a fix. Should this just be closed? Yes, the schema acceptance test has the cases which use dynamic schema file reload. Thus, it's covered by them. I'll mark verified and closed.
An advisory has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on therefore solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHEA-2009-0455.html