Date: Sun, 30 May 2010 14:17:29 -0700 From: Doug Barton <dougb@FreeBSD.org> To: Norikatsu Shigemura <nork@FreeBSD.org> Cc: cvs-ports@FreeBSD.org, cvs-all@FreeBSD.org, ports-committers@FreeBSD.org Subject: Re: cvs commit: ports/databases Makefile ports/databases/flare Makefile distinfo pkg-descr pkg-plist ports/databases/flare/files flared.sh.in flarei.sh.in patch-etc-flared.conf patch-etc-flarei.conf patch-flared-flared.cc patch-flared-ini_option.cc ... Message-ID: <4C02D5E9.4000400@FreeBSD.org> In-Reply-To: <201005300441.o4U4foHS067210@repoman.freebsd.org> References: <201005300441.o4U4foHS067210@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------000804020303030006070407 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit There are numerous problems with the rc.d scripts for this port. The single biggest problem is that no new scripts should be added with the .sh suffix, as it's never installed that way anymore. The attached patch fixes the other problems, but I haven't changed the file names in order to make the changes obvious. There are several things in your scripts that are not going to work quite the way you think they will, as well as some deviations from our usual conventions. In addition to the other problems, I would highly urge you to change flare_conffile to be simply flare_config as it's otherwise used in the script. That matches our convention, and is simpler. I did not make that change in the attached patch. 1. Default variable settings need to happen after load_rc_config. In a lot of cases this isn't fatal, but this is particularly important when the values are going to used (like pidfile, config file, etc.). 2. Instead of (ab)using _flags to set the default/required options, command_args should be used for this purpose. 3. The _flags checking you do is fine, but we don't allow code to run unconditionally in rc.d scripts, so it needs to be enclosed in a start precmd. I've also simplified it a bit, and switched it to an error instead of a warning. 4. REQUIRE'ing flarei in flared is the right thing to do, the BEFORE in flarei is neither necessary nor desirable. Please test and commit these changes ASAP, as well as bumping PORTREVISION. hth, Doug -- ... and that's just a little bit of history repeating. -- Propellerheads Improve the effectiveness of your Internet presence with a domain name makeover! http://SupersetSolutions.com/ --------------000804020303030006070407 Content-Type: text/plain; name="flare-rcd.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="flare-rcd.diff" Index: flared.sh.in =================================================================== RCS file: /home/pcvs/ports/databases/flare/files/flared.sh.in,v retrieving revision 1.2 diff -u -r1.2 flared.sh.in --- flared.sh.in 30 May 2010 15:05:18 -0000 1.2 +++ flared.sh.in 30 May 2010 20:58:26 -0000 @@ -1,54 +1,42 @@ #!/bin/sh -# + # $FreeBSD: ports/databases/flare/files/flared.sh.in,v 1.2 2010/05/30 15:05:18 nork Exp $ # # PROVIDE: flared # REQUIRE: LOGIN flarei -# -flared_enable=${flared_enable-"NO"} -flared_config=${flared_conffile-"%%PREFIX%%/etc/flared.conf"} -flared_pidfile=${flared_pidfile-"/var/run/flared.pid"} -flared_flags="--daemonize" . /etc/rc.subr name=flared rcvar=`set_rcvar` command=%%PREFIX%%/bin/${name} - extra_commands="reload" +start_precmd=${name}_prestart + +flared_prestart() +{ + case "$flared_flags" in + *-p\ *|*--pid\ *) + err 1 "\$flared_flags includes -p option." \ + "Please use \$flared_pidfile instead." + ;; + esac + + case "$flared_flags" in + *-f\ *|*--config\ *) + err 1 "\$flared_flags includes -f option." \ + "Please use \$flared_config instead." + ;; + esac +} -load_rc_config ${name} +load_rc_config $name -case "${flared_flags}" in -*-p\ *) - echo "Warning: \$flared_flags includes -p option." \ - "Please use \$flared_pidfile instead." - ;; -*--pid\ *) - echo "Warning: \$flared_flags includes -p option." \ - "Please use \$flared_pidfile instead." - ;; -*) - flared_flags="--pid ${flared_pidfile} ${flared_flags}" - ;; -esac - -case "${flared_flags}" in -*-f\ *) - echo "Warning: \$flared_flags includes -f option." \ - "Please use \$flared_config instead." - ;; -*--config\ *) - echo "Warning: \$flared_flags includes --config option." \ - "Please use \$flared_config instead." - ;; -*) - flared_flags="--config ${flared_config} ${flared_flags}" - ;; -esac +flared_enable=${flared_enable-"NO"} +flared_config=${flared_conffile-"%%PREFIX%%/etc/flared.conf"} -pidfile=${flared_pidfile} -required_files=${flared_conffile} +pidfile=${flared_pidfile-"/var/run/flared.pid"} +required_files=$flared_config +command_args="--config $flared_config --pid $pidfile --daemonize" run_rc_command "$1" Index: flarei.sh.in =================================================================== RCS file: /home/pcvs/ports/databases/flare/files/flarei.sh.in,v retrieving revision 1.2 diff -u -r1.2 flarei.sh.in --- flarei.sh.in 30 May 2010 15:05:18 -0000 1.2 +++ flarei.sh.in 30 May 2010 20:58:26 -0000 @@ -1,61 +1,48 @@ #!/bin/sh -# + # $FreeBSD: ports/databases/flare/files/flarei.sh.in,v 1.2 2010/05/30 15:05:18 nork Exp $ # # PROVIDE: flarei # REQUIRE: LOGIN -# BEFORE: flared -# -flarei_enable=${flarei_enable-"NO"} -flarei_config=${flarei_conffile-"%%PREFIX%%/etc/flarei.conf"} -flarei_pidfile=${flarei_pidfile-"/var/run/flarei.pid"} -flarei_flags="--daemonize" -flarei_sleepwait="2" . /etc/rc.subr name=flarei rcvar=`set_rcvar` command=%%PREFIX%%/bin/${name} - extra_commands="reload" +start_precmd=${name}_prestart start_postcmd="flarei_poststart" +flarei_prestart() +{ + case "$flarei_flags" in + *-p\ *|*--pid\ *) + err 1 "\$flarei_flags includes -p option." \ + "Please use \$flarei_pidfile instead." + ;; + esac + + case "$flarei_flags" in + *-f\ *|*--config\ *) + err 1 "\$flarei_flags includes -f option." \ + "Please use \$flarei_config instead." + ;; + esac +} + flarei_poststart () { - sleep "${flarei_sleepwait}" + sleep "$flarei_sleepwait" } -load_rc_config ${name} +load_rc_config $name -case "${flarei_flags}" in -*-p\ *) - echo "Warning: \$flarei_flags includes -p option." \ - "Please use \$flarei_pidfile instead." - ;; -*--pid\ *) - echo "Warning: \$flarei_flags includes -p option." \ - "Please use \$flarei_pidfile instead." - ;; -*) - flarei_flags="--pid ${flarei_pidfile} ${flarei_flags}" - ;; -esac - -case "${flarei_flags}" in -*-f\ *) - echo "Warning: \$flarei_flags includes -f option." \ - "Please use \$flarei_config instead." - ;; -*--config\ *) - echo "Warning: \$flarei_flags includes --config option." \ - "Please use \$flarei_config instead." - ;; -*) - flarei_flags="--config ${flarei_config} ${flarei_flags}" - ;; -esac +flarei_enable=${flarei_enable-"NO"} +flarei_config=${flarei_conffile-"%%PREFIX%%/etc/flarei.conf"} +flarei_sleepwait=${flarei_sleepwait-2} -pidfile=${flarei_pidfile} -required_files=${flarei_conffile} +pidfile=${flarei_pidfile-"/var/run/flarei.pid"} +required_files=$flarei_config +command_args="--config $flarei_config --pid $pidfile --daemonize" run_rc_command "$1" --------------000804020303030006070407--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4C02D5E9.4000400>