Re: [389-devel] Please Review: Fix parsing of start-slapd scripts
Nathan Kinder wrote: > On 03/31/2010 10:04 PM, Nathan Kinder wrote: >> On 03/31/2010 09:52 PM, Endi Sukma Dewata wrote: >> >>> - "Nathan Kinder" wrote: >>> >>> >>> >> The admin server CGIs parse the start-slapd scripts to determine the >> DS instance names. A recent format change to start-slapd caused >> this >> parsing to break. These patches make the instance name easier to >> parse from the script. One patch is for DS itself and one is for >> the >> Admin Server. >> >> >> > ack - much better > > > Thanks, but I need to nak my own patch since it's imcomplete. This isn't going to work well when upgrading an instance. We don't regenerate the start-slapd script when running 'setup-ds.pl -u'. This means that an upgraded instance will not work properly with any of the admin server CGIs that need to parse the instance name from start-slapd. This issue is already a problem not related to this patch, but it seems we should fix it along with this issue. I suppose the right thing to do is to make 'setup-ds.pl -u' generate a new start-slapd script for the existing instances as well as a new instance specific initconfig script if one doesn't exist. I think we need to avoid wiping out an existing instance specific sysconfig script since it may have been modified by an admin to add other stuff to it (like KRB5_KTNAME for Kerberos). Do you see any problems with this approach? > I've attached a new set of patches that implements the solution > outlined above. > > -NGK >>> Sorry that my changes caused this problem. Peace... :) >>> >>> >> Actually, a previous change of mine actually broke the upgrade case. >> You just broke the admin server parsing code. :) >> >>> Would it be better to have a pure configuration file (not script) in >>> the >>> instance directory that contains things like instance name, etc.? The >>> start-dirsrv is a script, and parsing a script without a proper >>> parser is >>> risky. Same thing with the sysconfig scripts. Even a slight change >>> in that >>> file could break some regular expressions in the Perl modules. What >>> do you >>> think? >>> >>> >> My previous change that breaks the upgrade case is trying to accomplish >> this. We previously had all sorts of paths in the start-slapd scripts >> which were parsed in various places. I created the instance specific >> config files in /etc/sysconfig that now contain the paths and other >> things needed to start an instance in an easily parsed format. The one >> wrinkle is that we need to know the instance name to know which >> sysconfig file to read since the file name is "dirsrv-". The >> admin server tries to get the instance name from the start script to >> find the sysconfig file so we can determine other things, such as the >> pid file location. The only other way I can think of to get the >> instance name would be to parse it out of the instance path that we use >> to locate the start-slapd script. This is usually something like >> "/usr/lib/dirsrv/slapd-". I think that my proposed change to >> the start-slapd format is less error prone since there is no complex >> parsing needed. >> >> -NGK ack >> >>> Thanks. >>> >>> -- >>> Endi S. Dewata >>> -- >>> 389-devel mailing list >>> 389-de...@lists.fedoraproject.org >>> https://admin.fedoraproject.org/mailman/listinfo/389-devel >>> >>> >> -- >> 389-devel mailing list >> 389-de...@lists.fedoraproject.org >> https://admin.fedoraproject.org/mailman/listinfo/389-devel >> > > > > -- > 389-devel mailing list > 389-de...@lists.fedoraproject.org > https://admin.fedoraproject.org/mailman/listinfo/389-devel -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please Review: Fix parsing of start-slapd scripts
On 04/01/2010 11:18 AM, Nathan Kinder wrote: On 03/31/2010 10:04 PM, Nathan Kinder wrote: On 03/31/2010 09:52 PM, Endi Sukma Dewata wrote: - "Nathan Kinder" wrote: The admin server CGIs parse the start-slapd scripts to determine the DS instance names. A recent format change to start-slapd caused this parsing to break. These patches make the instance name easier to parse from the script. One patch is for DS itself and one is for the Admin Server. ack - much better Thanks, but I need to nak my own patch since it's imcomplete. This isn't going to work well when upgrading an instance. We don't regenerate the start-slapd script when running 'setup-ds.pl -u'. This means that an upgraded instance will not work properly with any of the admin server CGIs that need to parse the instance name from start-slapd. This issue is already a problem not related to this patch, but it seems we should fix it along with this issue. I suppose the right thing to do is to make 'setup-ds.pl -u' generate a new start-slapd script for the existing instances as well as a new instance specific initconfig script if one doesn't exist. I think we need to avoid wiping out an existing instance specific sysconfig script since it may have been modified by an admin to add other stuff to it (like KRB5_KTNAME for Kerberos). Do you see any problems with this approach? I've attached a new set of patches that implements the solution outlined above. ack. --noriko smime.p7s Description: S/MIME Cryptographic Signature -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel
Re: [389-devel] Please Review: Fix parsing of start-slapd scripts
On 03/31/2010 10:04 PM, Nathan Kinder wrote: On 03/31/2010 09:52 PM, Endi Sukma Dewata wrote: - "Nathan Kinder" wrote: The admin server CGIs parse the start-slapd scripts to determine the DS instance names. A recent format change to start-slapd caused this parsing to break. These patches make the instance name easier to parse from the script. One patch is for DS itself and one is for the Admin Server. ack - much better Thanks, but I need to nak my own patch since it's imcomplete. This isn't going to work well when upgrading an instance. We don't regenerate the start-slapd script when running 'setup-ds.pl -u'. This means that an upgraded instance will not work properly with any of the admin server CGIs that need to parse the instance name from start-slapd. This issue is already a problem not related to this patch, but it seems we should fix it along with this issue. I suppose the right thing to do is to make 'setup-ds.pl -u' generate a new start-slapd script for the existing instances as well as a new instance specific initconfig script if one doesn't exist. I think we need to avoid wiping out an existing instance specific sysconfig script since it may have been modified by an admin to add other stuff to it (like KRB5_KTNAME for Kerberos). Do you see any problems with this approach? I've attached a new set of patches that implements the solution outlined above. -NGK Sorry that my changes caused this problem. Peace... :) Actually, a previous change of mine actually broke the upgrade case. You just broke the admin server parsing code. :) Would it be better to have a pure configuration file (not script) in the instance directory that contains things like instance name, etc.? The start-dirsrv is a script, and parsing a script without a proper parser is risky. Same thing with the sysconfig scripts. Even a slight change in that file could break some regular expressions in the Perl modules. What do you think? My previous change that breaks the upgrade case is trying to accomplish this. We previously had all sorts of paths in the start-slapd scripts which were parsed in various places. I created the instance specific config files in /etc/sysconfig that now contain the paths and other things needed to start an instance in an easily parsed format. The one wrinkle is that we need to know the instance name to know which sysconfig file to read since the file name is "dirsrv-". The admin server tries to get the instance name from the start script to find the sysconfig file so we can determine other things, such as the pid file location. The only other way I can think of to get the instance name would be to parse it out of the instance path that we use to locate the start-slapd script. This is usually something like "/usr/lib/dirsrv/slapd-". I think that my proposed change to the start-slapd format is less error prone since there is no complex parsing needed. -NGK Thanks. -- Endi S. Dewata -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel -- 389-devel mailing list 389-de...@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/389-devel >From 46b7eccd0859e7968f68fa7fc7dc55a1d19fb25f Mon Sep 17 00:00:00 2001 From: Nathan Kinder Date: Thu, 1 Apr 2010 11:12:02 -0700 Subject: [PATCH] Allow instance name to be parsed from start-slapd The admin server CGIs need to be able to easily parse the instance name from the start-slapd script. Recent format changes have caused the existing parsing to break, so this patch makes the parsing of the instance name easier. To deal with the change in start-slapd format for an upgraded instance, I have changed the setup code to regenerate all of the instance scripts during an upgrade instead of simply adding missing scripts. This is needed for any bug fix that modifies a script template to work for an upgraded instance. I also added code to write the instance sysconfig script during upgrade if it doesn't exist already. We don't want to overwrite this file if it already exists since it's designed for local changes to be made to it. --- ldap/admin/src/scripts/DSCreate.pm.in | 57 ++-- ldap/admin/src/scripts/DSUpdate.pm.in |9 +++- ldap/admin/src/scripts/template-start-slapd.in |3 +- 3 files changed, 42 insertions(+), 27 deletions(-) diff --git a/ldap/admin/src/scripts/DSCreate.pm.in b/ldap/admin/src/scripts/DSCreate.pm.in index 06b2d1f..09b7cd1 100644 --- a/ldap/admin/src/scripts/DSCreate.pm.in +++ b/ldap/admin/src/scripts/DSCreate.pm.in @@ -64,8 +64,8 @@ use Mozilla::LDAP::LDIF; use Exporter; @ISA = qw(Exporter); -...@export= qw(createDSInstance removeDSInstance setDefaults createInstanceScripts installSchema); -...@export_ok = qw(createDSInstance removeDSInstance setDefaults createInstanceScripts installSch
