On Fri, Jun 25, 2010 at 09:32:40AM -0600, Serge Dubrouski wrote: > Dejan - > > After a second thought I think that your way of handling checking > binaries is more appropriate since it allows that "case" statement > down the script work properly. I'd like to add a small change to > patch2 though. See attached.
I reintroduced the _default again, since it's referenced in the metadata. Everything pushed to the repository now. If you find more issues please send the patch against the repository. > pgsql3.patch has a mistypo: > > + if [ -n "$OCF_RESKEY_config" -a ! -f "$OCF_RESKEY_config" ]; then > + ocf_log err "the configuration file $OCF_RESKEY_config doesn't exist" > + return $OCF_ERR_INSTALLED > + if > > Should be > > + if [ -n "$OCF_RESKEY_config" -a ! -f "$OCF_RESKEY_config" ]; then > + ocf_log err "the configuration file $OCF_RESKEY_config doesn't exist" > + return $OCF_ERR_INSTALLED > + fi Thanks! It took me like 5 minutes to figure out where's the typo. Cheers, Dejan > On Fri, Jun 25, 2010 at 8:28 AM, Serge Dubrouski <[email protected]> wrote: > > Hi, Dejan - > > > > What's the point of introducing check_binary2 ? The old version of > > script used to use have_binary for checking binary tools but then > > Florian replaced it with check_binary and threw away "if" statements. > > Now you kind of reversing it. Why? > > > > > > On Fri, Jun 25, 2010 at 4:22 AM, Dejan Muhamedagic <[email protected]> > > wrote: > >> Hi, > >> > >> On Thu, Jun 24, 2010 at 11:59:55AM -0600, Serge Dubrouski wrote: > >>> Attached is an improved patch. > >>> > >>> 1. It parses unix_socket_directory out from PostgreSQL configuration > >>> file and used its value as a default value of OCF_RESKEY_socketdir. > >>> (Perl is used for that) > >>> > >>> 2. It creates OCF_RESKEY_socketdir if it doesn't exist otherwise > >>> checks if PostgreSQL DBA use can create files in that directory. > >>> > >>> 3. If OCF_RESKEY_socketdir is set bug OCF_RESKEY_pghost isn't it uses > >>> OCF_RESKEY_socketdir as -h parameter for psql command. Otherwise > >>> monitor function could fail because psql doesn't tolerate non-default > >>> socket directory and doesn't use posgresql.conf. > >>> > >>> 4. It replaced $(command) syntax with `command` syntax in 2 places > >>> just to make it consistent with the rest of the script. > >>> > >>> Perl parsing script may seem ugly but the rules of that config file > >>> are pretty wide open: equal sign is optional, spaces are optional, > >>> in-line comments are allowed. > >> > >> Thanks for the patch. I amended some parts of it (in > >> pgsql2.patch) and updated the validate action (pgsql3.patch). Can > >> you please test these. > >> > >> Thanks, > >> > >> Dejan > >> > >>> On Thu, Jun 24, 2010 at 8:37 AM, Serge Dubrouski <[email protected]> > >>> wrote: > >>> > That was I though to do with one exception: > >>> > > >>> > check_socket_dir() { > >>> > if [ ! -d "$1" ] > >>> > then > >>> > mkdir > >>> > chmod > >>> > chown > >>> > else > >>> > if ! touch $OCF_RESKEY_socketdir/test_file > >>> > then > >>> > ocf_log err "Can't create files in > >>> > $OCF_RESKEY_socketdir!" > >>> > exit > >>> > else > >>> > rm $OCF_RESKEY_socketdir/test_file > >>> > fi > >>> > fi > >>> > > >>> > I'll provide an appropriate patch a bit later. > >>> > > >>> > On Thu, Jun 24, 2010 at 8:33 AM, Dejan Muhamedagic > >>> > <[email protected]> wrote: > >>> >> On Wed, Jun 23, 2010 at 02:07:09PM +0200, Lars Ellenberg wrote: > >>> >>> On Tue, Jun 22, 2010 at 10:05:10AM -0600, Serge Dubrouski wrote: > >>> >>> > Yes it is possible to parse it from postgresql.conf of from provided > >>> >>> > config file. > >>> >>> > >>> >>> Then that should be done, IMO. > >>> >>> Otherwise it has to be changed in two places again, > >>> >>> and it will be forgotten in one or the other, > >>> >>> which will lead to very hard to debug misbehaviour. > >>> >> > >>> >> It would indeed be better to fetch it from the configuration > >>> >> file. However, there's also the concern that Serge mentioned on > >>> >> directories such as /tmp which should be handled differently. > >>> >> > >>> >> I'd suggest to drop the parameter and get it from the > >>> >> configuration file and create/modify the directory _only_ in case > >>> >> the directory doesn't exist. Codewise: > >>> >> > >>> >> check_socket_dir() { > >>> >> if [ ! -d "$1" ] > >>> >> then > >>> >> mkdir > >>> >> chmod > >>> >> chown > >>> >> fi > >>> >> } > >>> >> > >>> >> I think that this should be safe enough. > >>> >> > >>> >> Thanks, > >>> >> > >>> >> Dejan > >>> >> > >>> >>> > Attached is an improved patch with double quotes and error checks. > >>> >>> > >> > @@ -238,6 +248,11 @@ > >>> >>> > >> > ocf_log err "PostgreSQL can't write to the log file: > >>> >>> > >> > $OCF_RESKEY_logfile" > >>> >>> > >> > return $OCF_ERR_GENERIC > >>> >>> > >> > fi > >>> >>> > >> > + # Check if we need to create a socket directory > >>> >>> > >> > + if [ -n "$OCF_RESKEY_socketdir" ] > >>> >>> > >> > + then > >>> >>> > >> > + check_socket_dir $OCF_RESKEY_socketdir > >>> >>> > >>> >>> double quotes missing here already, btw. > >>> >>> and as OCF_RESKEY_socketdir is a "global" anyways, > >>> >>> why pass it as argument to subfunctions at all? > >>> >>> > >>> >>> -- > >>> >>> : Lars Ellenberg > >>> >>> : LINBIT | Your Way to High Availability > >>> >>> : DRBD/HA support and consulting http://www.linbit.com > >>> >>> > >>> >>> DRBD® and LINBIT® are registered trademarks of LINBIT, Austria. > >>> >>> _______________________________________________________ > >>> >>> Linux-HA-Dev: [email protected] > >>> >>> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > >>> >>> Home Page: http://linux-ha.org/ > >>> >> _______________________________________________________ > >>> >> Linux-HA-Dev: [email protected] > >>> >> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > >>> >> Home Page: http://linux-ha.org/ > >>> >> > >>> > > >>> > > >>> > > >>> > -- > >>> > Serge Dubrouski. > >>> > > >>> > >>> > >>> > >>> -- > >>> Serge Dubrouski. > >> > >>> --- a/heartbeat/pgsql 2010-06-24 08:40:18.000000000 -0600 > >>> +++ b/heartbeat/pgsql 2010-06-24 11:44:11.000000000 -0600 > >>> @@ -16,6 +16,38 @@ > >>> : ${OCF_FUNCTIONS_DIR=${OCF_ROOT}/resource.d/heartbeat} > >>> . ${OCF_FUNCTIONS_DIR}/.ocf-shellfuncs > >>> > >>> +# > >>> +# Get PostgreSQL Configuration parameter > >>> +# > >>> +get_pgsql_param() { > >>> + local config_file > >>> + local param_name > >>> + local param_value > >>> + > >>> + param_name=$1 > >>> + > >>> + #Check that config file exists > >>> + if [ -n $OCF_RESKEY_config ]; then > >>> + config=$OCF_RESKEY_pgdata/postgresql.conf > >>> + else > >>> + config=$OCF_RESKEY_config > >>> + fi > >>> + > >>> + if [ ! -f "$config" ]; then > >>> + ocf_log err "Cannot find configuration file $config" > >>> + exit $OCF_ERR_GENERIC > >>> + fi > >>> + > >>> + perl_code="if (/^\s*$param_name[\s=]+\s*(.*)$/) { > >>> + \$dir=\$1; > >>> + \$dir =~ s/\s*\#.*//; > >>> + \$dir =~ s/^'(\S*)'/\$1/; > >>> + print \$dir;}" > >>> + > >>> + param_value=`cat $config | perl -ne "$perl_code"` > >>> + echo "$param_value" > >>> +} > >>> + > >>> # Defaults > >>> OCF_RESKEY_pgctl_default=/usr/bin/pg_ctl > >>> OCF_RESKEY_psql_default=/usr/bin/psql > >>> @@ -27,7 +59,6 @@ > >>> OCF_RESKEY_start_opt_default="" > >>> OCF_RESKEY_pgdb_default=template1 > >>> OCF_RESKEY_logfile_default=/dev/null > >>> -OCF_RESKEY_socketdir_default="" > >>> OCF_RESKEY_stop_escalate_default=30 > >>> > >>> : ${OCF_RESKEY_pgctl=${OCF_RESKEY_pgctl_default}} > >>> @@ -40,9 +71,12 @@ > >>> : ${OCF_RESKEY_start_opt=${OCF_RESKEY_start_opt_default}} > >>> : ${OCF_RESKEY_pgdb=${OCF_RESKEY_pgdb_default}} > >>> : ${OCF_RESKEY_logfile=${OCF_RESKEY_logfile_default}} > >>> -: ${OCF_RESKEY_socketdir=${OCF_RESKEY_socketdir_default}} > >>> : ${OCF_RESKEY_stop_escalate=${OCF_RESKEY_stop_escalate_default}} > >>> > >>> +# $OCF_RESKEY_pgdata has to be initialized at this momemnt > >>> +OCF_RESKEY_socketdir_default=`get_pgsql_param unix_socket_directory` > >>> +: ${OCF_RESKEY_socketdir=${OCF_RESKEY_socketdir_default}} > >>> + > >>> usage() { > >>> cat <<EOF > >>> usage: $0 start|stop|status|monitor|meta-data|validate-all|methods > >>> @@ -248,10 +282,11 @@ > >>> ocf_log err "PostgreSQL can't write to the log file: > >>> $OCF_RESKEY_logfile" > >>> return $OCF_ERR_GENERIC > >>> fi > >>> - # Check if we need to create a socket directory > >>> + > >>> + # Check socket directory > >>> if [ -n "$OCF_RESKEY_socketdir" ] > >>> then > >>> - check_socket_dir $OCF_RESKEY_socketdir > >>> + check_socket_dir > >>> fi > >>> > >>> # Set options passed to pg_ctl > >>> @@ -385,6 +420,10 @@ > >>> psql_options="-p $OCF_RESKEY_pgport -U $OCF_RESKEY_pgdba > >>> $OCF_RESKEY_pgdb" > >>> if [ -n "$OCF_RESKEY_pghost" ]; then > >>> psql_options="$psql_options -h $OCF_RESKEY_pghost" > >>> + else > >>> + if [ -n "$OCF_RESKEY_socketdir" ]; then > >>> + psql_options="$psql_options -h $OCF_RESKEY_socketdir" > >>> + fi > >>> fi > >>> runasowner "$OCF_RESKEY_psql $psql_options -c 'select now();'" > >>> > >>> @@ -422,7 +461,7 @@ > >>> if [ ! -f "$1" ] > >>> then > >>> touch $1 > /dev/null 2>&1 > >>> - chown $OCF_RESKEY_pgdba:$(getent passwd $OCF_RESKEY_pgdba | cut > >>> -d ":" -f 4) $1 > >>> + chown $OCF_RESKEY_pgdba:`getent passwd $OCF_RESKEY_pgdba | cut > >>> -d ":" -f 4` $1 > >>> fi > >>> > >>> #Check if $OCF_RESKEY_pgdba can write to the log file > >>> @@ -434,27 +473,33 @@ > >>> return 0 > >>> } > >>> > >>> +# > >>> # Check socket directory > >>> +# > >>> check_socket_dir() { > >>> - if [ ! -d "$1" ] > >>> - then > >>> - if ! mkdir "$1" > >>> + if [ ! -d "$OCF_RESKEY_socketdir" ]; then > >>> + if ! mkdir "$OCF_RESKEY_socketdir"; then > >>> + ocf_log err "Cannot create directory $OCF_RESKEY_socketdir" > >>> + exit $OCF_ERR_GENERIC > >>> + fi > >>> + > >>> + if ! chown $OCF_RESKEY_pgdba:`getent passwd \ > >>> + $OCF_RESKEY_pgdba | cut -d ":" -f 4` "$OCF_RESKEY_socketdir" > >>> then > >>> - ocf_log err "Cannot create directory $1" > >>> + ocf_log err "Cannot change ownership for > >>> $OCF_RESKEY_socketdir" > >>> exit $OCF_ERR_GENERIC > >>> - fi > >>> - fi > >>> - > >>> - if ! chown $OCF_RESKEY_pgdba:$(getent passwd $OCF_RESKEY_pgdba | cut > >>> -d ":" -f 4) "$1" > >>> - then > >>> - ocf_log err "Cannot change ownership for $1" > >>> - exit $OCF_ERR_GENERIC > >>> - fi > >>> + fi > >>> > >>> - if ! chmod 2775 "$1" > >>> - then > >>> - ocf_log err "Cannot change permissions for $1" > >>> - exit $OCF_ERR_GENERIC > >>> + if ! chmod 2775 "$OCF_RESKEY_socketdir"; then > >>> + ocf_log err "Cannot change permissions for > >>> $OCF_RESKEY_socketdir" > >>> + exit $OCF_ERR_GENERIC > >>> + fi > >>> + else > >>> + if ! runasowner "touch $OCF_RESKEY_socketdir/test.$$"; then > >>> + ocf_log err "$OCF_RESKEY_pgdba cannot create files in > >>> $OCF_RESKEY_socketdir" > >>> + exit $OCF_ERR_GENERIC > >>> + fi > >>> + rm $OCF_RESKEY_socketdir/test.$$ > >>> fi > >>> } > >>> > >> > >>> _______________________________________________________ > >>> Linux-HA-Dev: [email protected] > >>> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > >>> Home Page: http://linux-ha.org/ > >> > >> > >> _______________________________________________________ > >> Linux-HA-Dev: [email protected] > >> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > >> Home Page: http://linux-ha.org/ > >> > >> > > > > > > > > -- > > Serge Dubrouski. > > > > > > -- > Serge Dubrouski. > --- a/heartbeat/pgsql 2010-06-25 09:22:42.000000000 -0600 > +++ b/heartbeat/pgsql 2010-06-25 09:22:29.000000000 -0600 > @@ -22,20 +22,19 @@ > get_pgsql_param() { > local config_file > local param_name > - local param_value > > param_name=$1 > > #Check that config file exists > - if [ -n $OCF_RESKEY_config ]; then > - config=$OCF_RESKEY_pgdata/postgresql.conf > - else > + if [ -n "$OCF_RESKEY_config" ]; then > config=$OCF_RESKEY_config > + else > + config=$OCF_RESKEY_pgdata/postgresql.conf > fi > > if [ ! -f "$config" ]; then > ocf_log err "Cannot find configuration file $config" > - exit $OCF_ERR_GENERIC > + return > fi > > perl_code="if (/^\s*$param_name[\s=]+\s*(.*)$/) { > @@ -44,8 +43,7 @@ > \$dir =~ s/^'(\S*)'/\$1/; > print \$dir;}" > > - param_value=`cat $config | perl -ne "$perl_code"` > - echo "$param_value" > + perl -ne "$perl_code" < $config > } > > # Defaults > @@ -74,8 +72,7 @@ > : ${OCF_RESKEY_stop_escalate=${OCF_RESKEY_stop_escalate_default}} > > # $OCF_RESKEY_pgdata has to be initialized at this momemnt > -OCF_RESKEY_socketdir_default=`get_pgsql_param unix_socket_directory` > -: ${OCF_RESKEY_socketdir=${OCF_RESKEY_socketdir_default}} > +: ${OCF_RESKEY_socketdir=`get_pgsql_param unix_socket_directory`} > > usage() { > cat <<EOF > _______________________________________________________ > Linux-HA-Dev: [email protected] > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > Home Page: http://linux-ha.org/ _______________________________________________________ Linux-HA-Dev: [email protected] http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/
