Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 06 Jun 2006 10:56:18 -0700
From:      Doug Barton <dougb@FreeBSD.org>
To:        Ying-Chieh Liao <ijliao@FreeBSD.org>
Cc:        "Andrey V. Elsukov" <bu7cher@yandex.ru>, "freebsd-ports@freebsd.org" <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)
Message-ID:  <4485C1C2.5030508@FreeBSD.org>
In-Reply-To: <200606060933.k569X7Al041179@repoman.freebsd.org>
References:  <200606060933.k569X7Al041179@repoman.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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" <bu7cher@yandex.ru> (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--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4485C1C2.5030508>