Bug 436837 - Dynamically reload schema via task interface
Dynamically reload schema via task interface
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Directory Server (Show other bugs)
1.1.0
All Linux
high Severity high
: ---
: ---
Assigned To: Noriko Hosoi
Chandrasekar Kannan
:
Depends On:
Blocks: 193799 249650 FDS112
  Show dependency treegraph
 
Reported: 2008-03-10 14:51 EDT by Noriko Hosoi
Modified: 2015-01-04 18:31 EST (History)
6 users (show)

See Also:
Fixed In Version: 8.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-29 19:02:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
cvs diffs (71.79 KB, patch)
2008-05-30 19:49 EDT, Noriko Hosoi
no flags Details | Diff
New file: ldapserver/ldap/servers/plugins/schema_reload/schema_reload.c (9.73 KB, text/plain)
2008-06-03 14:50 EDT, Noriko Hosoi
no flags Details
cvs diffs (48.28 KB, patch)
2008-06-04 14:18 EDT, Noriko Hosoi
no flags Details | Diff
cvs diffs (final) (136.80 KB, patch)
2008-06-04 18:10 EDT, Noriko Hosoi
no flags Details | Diff
cvs commit message (4.25 KB, text/plain)
2008-06-04 19:50 EDT, Noriko Hosoi
no flags Details
cvs diff slap.h backend.c (1.74 KB, patch)
2008-06-06 13:48 EDT, Noriko Hosoi
no flags Details | Diff
cvs commit message (658 bytes, text/plain)
2008-06-06 13:54 EDT, Noriko Hosoi
no flags Details
cvs diffs (4.82 KB, patch)
2008-06-10 14:37 EDT, Noriko Hosoi
no flags Details | Diff
cvs commit message (909 bytes, text/plain)
2008-06-10 14:51 EDT, Noriko Hosoi
no flags Details
cvs diff schema_reload.c (1.16 KB, patch)
2008-07-16 14:08 EDT, Noriko Hosoi
no flags Details | Diff

  None (edit)
Description Noriko Hosoi 2008-03-10 14:51:12 EDT
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);
    }
Comment 1 Noriko Hosoi 2008-05-30 19:49:44 EDT
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.
Comment 2 Noriko Hosoi 2008-06-03 14:50:06 EDT
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.
Comment 3 Noriko Hosoi 2008-06-04 14:18:21 EDT
Created attachment 308379 [details]
cvs diffs
Comment 4 Noriko Hosoi 2008-06-04 14:25:25 EDT
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
Comment 5 Nathan Kinder 2008-06-04 17:00:08 EDT
Looks good, but I don't think you want to change SLAPD_VENDOR_NAME and
SLAPD_VERSION_STR in slap.h.
Comment 6 Noriko Hosoi 2008-06-04 18:10:17 EDT
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.
Comment 7 Noriko Hosoi 2008-06-04 19:50:11 EDT
Created attachment 308403 [details]
cvs commit message

Reviewed by Nathan (Thanks a lot!)

Checked in into CVS HEAD.
Comment 8 Noriko Hosoi 2008-06-04 19:54:10 EDT
*** Bug 193799 has been marked as a duplicate of this bug. ***
Comment 9 Rich Megginson 2008-06-06 11:16:44 EDT
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.
Comment 10 Noriko Hosoi 2008-06-06 13:02:00 EDT
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.
Comment 11 Rich Megginson 2008-06-06 13:13:13 EDT
(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.

Comment 12 Noriko Hosoi 2008-06-06 13:48:09 EDT
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.
Comment 13 Noriko Hosoi 2008-06-06 13:50:59 EDT
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
Comment 14 Noriko Hosoi 2008-06-06 13:54:45 EDT
Created attachment 308555 [details]
cvs commit message

Thanks to Rich for his reviews and comments.

Checked in into CVS HEAD.
Comment 15 Noriko Hosoi 2008-06-10 14:37:01 EDT
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.
Comment 16 Noriko Hosoi 2008-06-10 14:51:16 EDT
Created attachment 308848 [details]
cvs commit message

Reviewed by Nathan (Thank you!!)

Checked in into CVS HEAD.
Comment 17 Noriko Hosoi 2008-07-16 14:08:07 EDT
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
Comment 18 Jenny Galipeau 2009-03-12 13:42:59 EDT
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?
Comment 19 Noriko Hosoi 2009-03-12 14:02:55 EDT
(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.
Comment 20 Chandrasekar Kannan 2009-04-29 19:02:54 EDT
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

Note You need to log in before you can comment on or make changes to this bug.