From owner-freebsd-ports@FreeBSD.ORG Tue Jun 6 17:56:22 2006 Return-Path: X-Original-To: freebsd-ports@freebsd.org Delivered-To: freebsd-ports@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id D3A2F16ADC1 for ; Tue, 6 Jun 2006 17:56:22 +0000 (UTC) (envelope-from dougb@FreeBSD.org) Received: from mail2.fluidhosting.com (mx21.fluidhosting.com [204.14.89.4]) by mx1.FreeBSD.org (Postfix) with SMTP id 2056643D55 for ; Tue, 6 Jun 2006 17:56:21 +0000 (GMT) (envelope-from dougb@FreeBSD.org) Received: (qmail 6188 invoked by uid 399); 6 Jun 2006 17:56:21 -0000 Received: from localhost (HELO ?192.168.0.3?) (dougb@dougbarton.us@127.0.0.1) by localhost with SMTP; 6 Jun 2006 17:56:21 -0000 Message-ID: <4485C1C2.5030508@FreeBSD.org> Date: Tue, 06 Jun 2006 10:56:18 -0700 From: Doug Barton Organization: http://www.FreeBSD.org/ User-Agent: Thunderbird 1.5.0.4 (X11/20060604) MIME-Version: 1.0 To: Ying-Chieh Liao References: <200606060933.k569X7Al041179@repoman.freebsd.org> In-Reply-To: <200606060933.k569X7Al041179@repoman.freebsd.org> X-Enigmail-Version: 0.94.0.0 Content-Type: multipart/mixed; boundary="------------060504090209070307070903" Cc: "Andrey V. Elsukov" , "freebsd-ports@freebsd.org" Subject: New cvsd rc.d script (Was: Re: cvs commit: ports/devel/cvsd Makefile ports/devel/cvsd/files cvsd.sh.in extra-cvsd-buildroot.in) X-BeenThere: freebsd-ports@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting software to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Jun 2006 17:56:27 -0000 This is a multi-part message in MIME format. --------------060504090209070307070903 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Ying-Chieh Liao wrote: > ijliao 2006-06-06 09:33:07 UTC > > FreeBSD ports repository > > Modified files: > devel/cvsd Makefile > Added files: > devel/cvsd/files cvsd.sh.in extra-cvsd-buildroot.in > Log: > Take maintainership > Add rc.subr support > > PR: 98582 http://www.FreeBSD.org/cgi/query-pr.cgi?pr=98582 > Submitted by: "Andrey V. Elsukov" (new maintainer) > > Revision Changes Path > 1.39 +12 -5 ports/devel/cvsd/Makefile > 1.1 +52 -0 ports/devel/cvsd/files/cvsd.sh.in (new) > 1.1 +42 -0 ports/devel/cvsd/files/extra-cvsd-buildroot.in (new) > > http://www.FreeBSD.org/cgi/cvsweb.cgi/ports/devel/cvsd/Makefile.diff?&r1=1.38&r2=1.39&f=h > http://www.FreeBSD.org/cgi/cvsweb.cgi/ports/devel/cvsd/files/cvsd.sh.in > http://www.FreeBSD.org/cgi/cvsweb.cgi/ports/devel/cvsd/files/extra-cvsd-buildroot.in First off let me thank you for taking the initiative to move this port to the rc.d framework. I have some suggestions for you, and I hope that you will not take them as a criticism in any way. This script provides a good educational opportunity, so although I'm offering a fairly extensive patch here, it's still based on your good work. In addition to the attached patch, I'm going to provide a bit of commentary here to explain my suggestions. First, the name of the .in file and the USE_RC_SUBR variable should be cvsd, not cvsd.sh. The Porter's Handbook had the .sh example until just a second ago when I fixed it, so I apologize if you used that as a reference. It's not a big enough problem to justify changing the name now, as bsd.port.mk will do the right thing. Just something to keep in mind for future reference. Index: cvsd.sh.in =================================================================== RCS file: /home/pcvs/ports/devel/cvsd/files/cvsd.sh.in,v retrieving revision 1.1 diff -u -r1.1 cvsd.sh.in --- cvsd.sh.in 6 Jun 2006 09:33:07 -0000 1.1 +++ cvsd.sh.in 6 Jun 2006 17:37:03 -0000 @@ -2,24 +2,22 @@ # $FreeBSD: # # PROVIDE: cvsd -# REQUIRE: NETWORKING -# KEYWORD: nojail +# REQUIRE: LOGIN Most services should REQUIRE LOGIN unless there is a good reason to have them start sooner. In this case, your service will almost certainly need LOGIN as it runs with an unprivileged user account. +# KEYWORD: nojail shutdown If you're starting a daemon, you should include the shutdown KEYWORD so that it will be disabled in an orderly fashion when the system is shut down. . %%RC_SUBR%% name="cvsd" -rcvar=`set_rcvar` -command="%%PREFIX%%/sbin/$name" - -load_rc_config $name +rcvar=${name}_enable Minor issue, but saves one layer of indirection. -: ${cvsd_enable="NO"} -: ${cvsd_config="%%PREFIX%%/etc/$name/$name.conf"} +command="%%PREFIX%%/sbin/$name" +command_args='-f $cvsd_config' +required_files=$cvsd_config -command_args="-f $cvsd_config" +start_precmd=${name}_prestart +stop_postcmd=${name}_poststop -start_precmd="cvsd_prestart" -stop_postcmd="cvsd_poststop" +osreldate=`sysctl -n kern.osreldate` These changes are more in keeping with typical rc.d style. Also, one little shell trick for you. If you enclose the name of the variable you want to use in single quotes as I did above for command_args, you can define command_args before cvsd_config gets define, and the variable will be filled in at the appropriate time in run_rc_command. cvsd_prestart() { @@ -29,6 +27,16 @@ devfs -m $jail/dev rule apply path null unhide devfs -m $jail/dev rule apply path zero unhide fi + + jail=`sed -n 's/^ *RootJail *\([^ ]*\) *$/\1/p' < $cvsd_config` + if [ -z "$jail" ]; then + err 1 "RootJail is not specified in $cvsd_config" + fi + + pidfile=`sed -n 's/^ *PidFile *\([^ ]*\) *$/\1/p' < $cvsd_config` + if [ -z "$pidfile" ]; then + err 1 "PidFile is not specified in $cvsd_config" + fi Two things here, first you should never perform script actions unconditionally in an rc.d script. The test for this is to run the script with for example './cvsd rcvar' and make sure that the only output will be the values of the variables. Second, the -z test is the preferred way to test if a variable is empty. I hope this is useful for you. If you approve the changes, I'll be glad to commit them for you, or you can submit a new PR. Regards, Doug -- This .signature sanitized for your protection --------------060504090209070307070903 Content-Type: text/plain; name="cvsd.sh.in.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="cvsd.sh.in.diff" Index: cvsd.sh.in =================================================================== RCS file: /home/pcvs/ports/devel/cvsd/files/cvsd.sh.in,v retrieving revision 1.1 diff -u -r1.1 cvsd.sh.in --- cvsd.sh.in 6 Jun 2006 09:33:07 -0000 1.1 +++ cvsd.sh.in 6 Jun 2006 17:37:03 -0000 @@ -2,24 +2,22 @@ # $FreeBSD: # # PROVIDE: cvsd -# REQUIRE: NETWORKING -# KEYWORD: nojail +# REQUIRE: LOGIN +# KEYWORD: nojail shutdown . %%RC_SUBR%% name="cvsd" -rcvar=`set_rcvar` -command="%%PREFIX%%/sbin/$name" - -load_rc_config $name +rcvar=${name}_enable -: ${cvsd_enable="NO"} -: ${cvsd_config="%%PREFIX%%/etc/$name/$name.conf"} +command="%%PREFIX%%/sbin/$name" +command_args='-f $cvsd_config' +required_files=$cvsd_config -command_args="-f $cvsd_config" +start_precmd=${name}_prestart +stop_postcmd=${name}_poststop -start_precmd="cvsd_prestart" -stop_postcmd="cvsd_poststop" +osreldate=`sysctl -n kern.osreldate` cvsd_prestart() { @@ -29,6 +27,16 @@ devfs -m $jail/dev rule apply path null unhide devfs -m $jail/dev rule apply path zero unhide fi + + jail=`sed -n 's/^ *RootJail *\([^ ]*\) *$/\1/p' < $cvsd_config` + if [ -z "$jail" ]; then + err 1 "RootJail is not specified in $cvsd_config" + fi + + pidfile=`sed -n 's/^ *PidFile *\([^ ]*\) *$/\1/p' < $cvsd_config` + if [ -z "$pidfile" ]; then + err 1 "PidFile is not specified in $cvsd_config" + fi } cvsd_poststop() @@ -38,15 +46,9 @@ fi } -jail=`sed -n 's/^ *RootJail *\([^ ]*\) *$/\1/p' < $cvsd_config` -pidfile=`sed -n 's/^ *PidFile *\([^ ]*\) *$/\1/p' < $cvsd_config` -osreldate=`sysctl -n kern.osreldate` -if [ "$jail" = "X$jail" ]; then - err 1 "RootJail is not specified in $cvsd_config" -fi -if [ "$pidfile" = "X$pidfile" ]; then - err 1 "PidFile is not specified in $cvsd_config" -fi +load_rc_config $name -run_rc_command "$1" +: ${cvsd_enable="NO"} +: ${cvsd_config="%%PREFIX%%/etc/$name/$name.conf"} +run_rc_command "$1" --------------060504090209070307070903--