Date: Sat, 11 Feb 2012 01:38:21 -0800 From: Doug Barton <dougb@FreeBSD.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: Xin LI <delphij@delphij.net>, freebsd-rc@FreeBSD.org Subject: Re: Bringing sanity to the RPC/NFS related scripts Message-ID: <4F36370D.7@FreeBSD.org> In-Reply-To: <20120210110356.GA6723@stack.nl> References: <4F322437.5030100@FreeBSD.org> <20120208230852.GA83950@stack.nl> <4F33031E.7000507@FreeBSD.org> <20120210110356.GA6723@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------030303010403040105070906 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit I'm copying delphij since this is in part related to the patch he posted back in August. I had a chance to review that in detail (thanks Jilles for the pointer) and I agree that his approach is the right one, it just doesn't go far enough. :) The attached patch encompasses his work (with modifications as described below), my previous patch, the idea I had for the always_force_depends knob, and the change requested by Jilles. I also sorted the first few vars in the yp* scripts ... they were in various weird orders, with the problem being that rcvar was often set after load_rc_config. The idea of putting the logic for testing whether or not the service is enabled and the forcestatus bit into a function is definitely the right way to go. delphij's original patch cut out a lot of code from the rc.d scripts, and this patch cuts out even more. Hopefully making force_depend both more useful and easier to use will encourage more use of it. I did take one slightly different choice with the syntax for overloading force_depend .... rather than having to feed it the full rcvar (like foo_bar_enable) I changed it to leave off the _enable bit in the argument since it's redundant to include it every time. Doug On 02/10/2012 03:03, Jilles Tjoelker wrote: > On Wed, Feb 08, 2012 at 03:19:58PM -0800, Doug Barton wrote: >> Thanks for the review, comments below, with snipping. > >> On 02/08/2012 15:08, Jilles Tjoelker wrote: >>> On Tue, Feb 07, 2012 at 11:28:55PM -0800, Doug Barton wrote: > >>>> and b) the test for "is it running?" is conditional on it not >>>> being _enable'd, which is counterproductive for a couple of reasons. (I >>>> can elaborate if necessary, but hopefully it's obvious?) So I'd like to >>>> propose removing the _enable check from all the relevant scripts that >>>> have this force_depend capability. For users who already have _enable >>>> for these services it will cause one extra call to forcestatus for them, >>>> but given that rc.d currently has no other way to ensure that required >>>> dependencies are running, I think it's worth it. > >>> This was discussed in August 2011 but no patch was committed. See >>> http://lists.freebsd.org/pipermail/freebsd-rc/2011-August/002412.html > >>> The existing code makes a lot of sense for the case [ -n "${rc_fast}" ] >>> (avoiding unnecessary slow checks for processes at boot) but may be less >>> convenient for starting such services manually. The patch appears to fix >>> the manual start case without slowing down boot, unlike bluntly removing >>> checkyesno which will slow down boot. > >> I understand the motivation not to slow down the boot, however the >> problems we're seeing with "random" statd failures are at boot time. So >> perhaps the right answer is to include the fast_depend idea but with an >> override to always do the check. > > Hmm, so statd sometimes does not feel like starting the first time, but > is willing the second time? That would be a bug that should not be > worked around with that extra check. > >> Also, I've only taken a cursory glance at the patch (I'll have more time >> to review it later) but it seems to me that rather than introducing a >> new function it would be better to have force_depend test for $rc_fast >> (and it could then also test for the override knob I'd like to add). Any >> objections to that? > > No objection. > >>>> Index: mountd >>>> =================================================================== >>>> --- mountd (revision 231185) >>>> +++ mountd (working copy) >>>> [snip] >>>> @@ -49,7 +47,6 @@ >>>> >>>> rm -f /var/db/mountdtab >>>> ( umask 022 ; > /var/db/mountdtab ) >>>> - return 0 >>>> } >>>> >>>> load_rc_config $name > >>> Note that this changes the return value of mountd_precmd if >>> /var/db/mountdtab cannot be created. Is this deliberate? > >> Yes, since mountd relies on that. Do you think it's overkill? Perhaps it >> would be better to add an '|| err ...' to explain the failure? > > That's probably safer, since a subsequent modification may not take into > account that the redirection is deliberately last. > > If this change is conscious, that's fine. > -- It's always a long day; 86400 doesn't fit into a short. Breadth of IT experience, and depth of knowledge in the DNS. Yours for the right price. :) http://SupersetSolutions.com/ --------------030303010403040105070906 Content-Type: text/plain; name="etc-rpc-nfs.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="etc-rpc-nfs.diff" Index: share/man/man5/rc.conf.5 =================================================================== --- share/man/man5/rc.conf.5 (revision 231503) +++ share/man/man5/rc.conf.5 (working copy) @@ -24,7 +24,7 @@ .\" .\" $FreeBSD$ .\" -.Dd February 8, 2012 +.Dd February 11, 2012 .Dt RC.CONF 5 .Os .Sh NAME @@ -149,6 +149,19 @@ adequate provisions to recover from a failed boot (such as physical contact with the machine, or reliable remote console access). +.It Va always_force_depends +.Pq Vt bool +Various +.Pa rc.d +scripts use the force_depend function to check whether required +services are already running, and to start them if necessary. +By default during boot time this check is bypassed if the +required service is enabled in +.Pa /etc/rc.conf[.local] . +Setting this option will bypass that check at boot time and +always test whether or not the service is actually running. +Enabling this option is likely to increase your boot time if +services are enabled that utilize the force_depend check. .It Va swapfile .Pq Vt str If set to Index: etc/defaults/rc.conf =================================================================== --- etc/defaults/rc.conf (revision 231503) +++ etc/defaults/rc.conf (working copy) @@ -29,6 +29,8 @@ # stages of the boot process. Make sure you know # the ramifications if you change this. # See rc.conf(5) for more details. +always_force_depends="NO" # Set to check that indicated dependencies are + # running during boot (can increase boot time). swapfile="NO" # Set to name of swapfile if aux swapfile desired. apm_enable="NO" # Set to YES to enable APM BIOS functions (or NO). Index: etc/rc.d/ypset =================================================================== --- etc/rc.d/ypset (revision 231503) +++ etc/rc.d/ypset (working copy) @@ -11,25 +11,20 @@ name="ypset" rcvar="nis_ypset_enable" + +load_rc_config $name + command="/usr/sbin/${name}" -start_precmd="ypset_precmd" -load_rc_config $name command_args="${nis_ypset_flags}" +start_precmd="ypset_precmd" + ypset_precmd() { local _domain - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi - if ! checkyesno nis_client_enable && \ - ! /etc/rc.d/ypbind forcestatus 1>/dev/null 2>&1 - then - force_depend ypbind || return 1 - fi + force_depend rpcbind || return 1 + force_depend ypbind nis_client || return 1 _domain=`domainname` if [ -z "$_domain" ]; then Index: etc/rc.d/mountd =================================================================== --- etc/rc.d/mountd (revision 231503) +++ etc/rc.d/mountd (working copy) @@ -19,11 +19,7 @@ mountd_precmd() { - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi + force_depend rpcbind || return 1 # mountd flags will differ depending on rc.conf settings # @@ -48,8 +44,8 @@ fi rm -f /var/db/mountdtab - ( umask 022 ; > /var/db/mountdtab ) - return 0 + ( umask 022 ; > /var/db/mountdtab ) || + err 1 'Cannot create /var/db/mountdtab' } load_rc_config $name Index: etc/rc.d/yppasswdd =================================================================== --- etc/rc.d/yppasswdd (revision 231503) +++ etc/rc.d/yppasswdd (working copy) @@ -11,27 +11,22 @@ . /etc/rc.subr name="yppasswdd" -command="/usr/sbin/rpc.${name}" -start_precmd="yppasswdd_precmd" +rcvar="nis_yppasswdd_enable" load_rc_config $name -rcvar="nis_yppasswdd_enable" + +command="/usr/sbin/rpc.${name}" command_args="${nis_yppasswdd_flags}" +start_precmd="yppasswdd_precmd" + yppasswdd_precmd() { local _domain - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi - if ! checkyesno nis_server_enable && \ - ! /etc/rc.d/ypserv forcestatus 1>/dev/null 2>&1 - then - force_depend ypserv || return 1 - fi + force_depend rpcbind || return 1 + force_depend ypserv nis_server || return 1 + _domain=`domainname` if [ -z "$_domain" ]; then warn "NIS domainname(1) is not set." Index: etc/rc.d/ypupdated =================================================================== --- etc/rc.d/ypupdated (revision 231503) +++ etc/rc.d/ypupdated (working copy) @@ -11,6 +11,9 @@ name="ypupdated" rcvar="rpc_ypupdated_enable" + +load_rc_config $name + command="/usr/sbin/rpc.${name}" start_precmd="rpc_ypupdated_precmd" @@ -18,16 +21,8 @@ { local _domain - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi - if ! checkyesno nis_server_enable && \ - ! /etc/rc.d/ypserv forcestatus 1>/dev/null 2>&1 - then - force_depend ypserv || return 1 - fi + force_depend rpcbind || return 1 + force_depend ypserv nis_server || return 1 _domain=`domainname` if [ -z "$_domain" ]; then @@ -36,5 +31,4 @@ fi } -load_rc_config $name run_rc_command "$1" Index: etc/rc.d/nfsd =================================================================== --- etc/rc.d/nfsd (revision 231503) +++ etc/rc.d/nfsd (working copy) @@ -48,31 +48,15 @@ if checkyesno nfsv4_server_enable; then sysctl vfs.nfsd.server_max_nfsvers=4 > /dev/null - if ! checkyesno nfsuserd_enable && \ - ! /etc/rc.d/nfsuserd forcestatus 1>/dev/null 2>&1 - then - if ! force_depend nfsuserd; then - err 1 "Cannot run nfsuserd" - fi - fi + force_depend nfsuserd || err 1 "Cannot run nfsuserd" else echo 'NFSv4 is disabled' sysctl vfs.nfsd.server_max_nfsvers=3 > /dev/null fi fi - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi - - if ! checkyesno mountd_enable && \ - ! /etc/rc.d/mountd forcestatus 1>/dev/null 2>&1 - then - force_depend mountd || return 1 - fi - return 0 + force_depend rpcbind || return 1 + force_depend mountd || return 1 } run_rc_command "$1" Index: etc/rc.d/ypbind =================================================================== --- etc/rc.d/ypbind (revision 231503) +++ etc/rc.d/ypbind (working copy) @@ -11,22 +11,20 @@ . /etc/rc.subr name="ypbind" -command="/usr/sbin/${name}" -start_precmd="ypbind_precmd" +rcvar="nis_client_enable" load_rc_config $name -rcvar="nis_client_enable" + +command="/usr/sbin/${name}" command_args="${nis_client_flags}" +start_precmd="ypbind_precmd" + ypbind_precmd() { local _domain - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi + force_depend rpcbind || return 1 _domain=`domainname` if [ -z "$_domain" ]; then Index: etc/rc.d/ypserv =================================================================== --- etc/rc.d/ypserv (revision 231503) +++ etc/rc.d/ypserv (working copy) @@ -11,21 +11,20 @@ name="ypserv" rcvar="nis_server_enable" -command="/usr/sbin/${name}" -start_precmd="ypserv_prestart" load_rc_config $name + +command="/usr/sbin/${name}" command_args="${nis_server_flags}" +start_precmd="ypserv_prestart" + ypserv_prestart() { local _domain - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi + force_depend rpcbind || return 1 + _domain=`domainname` if [ -z "$_domain" ]; then warn "NIS domainname(1) is not set." Index: etc/rc.d/ypxfrd =================================================================== --- etc/rc.d/ypxfrd (revision 231503) +++ etc/rc.d/ypxfrd (working copy) @@ -11,25 +11,20 @@ name="ypxfrd" rcvar="nis_ypxfrd_enable" + +load_rc_config $name + command="/usr/sbin/rpc.${name}" -start_precmd="ypxfrd_precmd" -load_rc_config $name command_args="${nis_ypxfrd_flags}" +start_precmd="ypxfrd_precmd" + ypxfrd_precmd() { local _domain - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi - if ! checkyesno nis_server_enable && \ - ! /etc/rc.d/ypserv forcestatus 1>/dev/null 2>&1 - then - force_depend ypserv || return 1 - fi + force_depend rpcbind || return 1 + force_depend ypserv nis_server || return 1 _domain=`domainname` if [ -z "$_domain" ]; then Index: etc/rc.d/amd =================================================================== --- etc/rc.d/amd (revision 231503) +++ etc/rc.d/amd (working copy) @@ -19,16 +19,9 @@ amd_precmd() { - if ! checkyesno nfs_client_enable; then - force_depend nfsclient || return 1 - fi + force_depend nfsclient nfs_client || return 1 + force_depend rpcbind || return 1 - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi - case ${amd_map_program} in [Nn][Oo] | '') ;; @@ -49,7 +42,6 @@ command_args="> /var/run/amd.pid 2> /dev/null" ;; esac - return 0 } load_rc_config $name Index: etc/rc.d/keyserv =================================================================== --- etc/rc.d/keyserv (revision 231503) +++ etc/rc.d/keyserv (working copy) @@ -19,13 +19,7 @@ keyserv_prestart() { - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || return 1 - fi - - return 0 + force_depend rpcbind || return 1 } load_rc_config $name Index: etc/rc.d/apmd =================================================================== --- etc/rc.d/apmd (revision 231503) +++ etc/rc.d/apmd (working copy) @@ -19,24 +19,18 @@ { case `${SYSCTL_N} hw.machine_arch` in i386) - # Enable apm if it is not already enabled - if ! checkyesno apm_enable && \ - ! /etc/rc.d/apm forcestatus 1>/dev/null 2>&1 - then - force_depend apm || return 1 - fi + force_depend apm || return 1 # Warn user about acpi apm compatibility support which # does not work with apmd. if [ ! -e /dev/apmctl ]; then - warn "/dev/apmctl not found; kernel is missing apm(4)" + warn "/dev/apmctl not found; kernel is missing apm(4)" fi ;; *) return 1 ;; esac - return 0 } load_rc_config $name Index: etc/rc.d/lockd =================================================================== --- etc/rc.d/lockd (revision 231503) +++ etc/rc.d/lockd (working copy) @@ -15,28 +15,16 @@ rcvar=rpc_lockd_enable command="/usr/sbin/rpc.${name}" start_precmd='lockd_precmd' -stop_precmd='checkyesno nfs_server_enable || checkyesno nfs_client_enable' -status_precmd=$stop_precmd # Make sure that we are either an NFS client or server, and that we get # the correct flags from rc.conf(5). # lockd_precmd() { - local ret - ret=0 - - if ! checkyesno nfs_server_enable && ! checkyesno nfs_client_enable - then - ret=1 - fi - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || ret=1 - fi + force_depend rpcbind || return 1 + force_depend statd rpc_statd || return 1 + rc_flags=${rpc_lockd_flags} - return ${ret} } load_rc_config $name Index: etc/rc.d/statd =================================================================== --- etc/rc.d/statd (revision 231503) +++ etc/rc.d/statd (working copy) @@ -15,28 +15,15 @@ rcvar=rpc_statd_enable command="/usr/sbin/rpc.${name}" start_precmd='statd_precmd' -stop_precmd='checkyesno nfs_server_enable || checkyesno nfs_client_enable' -status_precmd=$stop_precmd # Make sure that we are either an NFS client or server, and that we get # the correct flags from rc.conf(5). # statd_precmd() { - local ret - ret=0 - - if ! checkyesno nfs_server_enable && ! checkyesno nfs_client_enable - then - ret=1 - fi - if ! checkyesno rpcbind_enable && \ - ! /etc/rc.d/rpcbind forcestatus 1>/dev/null 2>&1 - then - force_depend rpcbind || ret=1 - fi + force_depend rpcbind || return 1 + rc_flags=${rpc_statd_flags} - return ${ret} } load_rc_config $name Index: etc/rc.subr =================================================================== --- etc/rc.subr (revision 231503) +++ etc/rc.subr (working copy) @@ -71,22 +71,29 @@ } # -# force_depend script +# force_depend script [rcvar] # Force a service to start. Intended for use by services -# to resolve dependency issues. It is assumed the caller -# has check to make sure this call is necessary +# to resolve dependency issues. # $1 - filename of script, in /etc/rc.d, to run +# $2 - name of the script's rcvar (minus the _enable) # force_depend() { + local _depend _dep_rcvar + _depend="$1" + _dep_rcvar="${2:-$1}_enable" + [ -n "$rc_fast" ] && ! checkyesno always_force_depends && + checkyesno $_dep_rcvar && return 0 + + /etc/rc.d/${_depend} forcestatus >/dev/null 2>&1 && return 0 + info "${name} depends on ${_depend}, which will be forced to start." if ! /etc/rc.d/${_depend} forcestart; then warn "Unable to force ${_depend}. It may already be running." return 1 fi - return 0 } # --------------030303010403040105070906--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4F36370D.7>