Hi, On Fri, Jun 25, 2010 at 08:28:54AM -0600, Serge Dubrouski 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?
Didn't see that changeset, but using check_binary here is wrong, because it immediately exits instead of letting the caller decide what to do. The earlier version was correct, Florian probably misunderstood parts of the code. That changeset contains an unrelated change: -OCF_RESKEY_start_opt_default="-p $OCF_RESKEY_pgport_default" +OCF_RESKEY_start_opt_default="" Looks OK, but can you please check too. Oh, I see now that start_opt is also repeated twice in the metadata, can you please choose one and send a patch. Thanks, Dejan > 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. > _______________________________________________________ > 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/
