rc.subr / rc.d/sshd patch for review

[Brooks Davis]

On Thu, Mar 30, 2006 at 12:48:24AM +0100, Florent Thoumie wrote:
> On Mar 28, 2006, at 6:30 PM, Florent Thoumie wrote:
> 
> >On Mar 28, 2006, at 6:08 PM, Brooks Davis wrote:
> >
> >>On Tue, Mar 28, 2006 at 03:38:35PM +0100, Florent Thoumie wrote:
> >>>On Mon, 2006-03-27 at 10:37 -0800, Brooks Davis wrote:
> >>>>On Mon, Mar 27, 2006 at 01:06:30PM +0100, Florent Thoumie wrote:
> >>>>>On Sat, 2006-03-25 at 11:06 +0000, Florent Thoumie wrote:
> >>>>>>On Mar 24, 2006, at 8:56 PM, Brooks Davis wrote:
> >>>>>>
> >>>>>>>On Fri, Mar 24, 2006 at 12:15:49PM +0000, Florent Thoumie wrote:
> >>>>>>>>This is based on Oliver's patch for rc.d/sshd that can be  
> >>>>>>>>found in
> >>>>>>>>Gnats.
> >>>>>>>>
> >>>>>>>>In load_rc_config, I'm extracting prefix from ${command} (or
> >>>>>>>>${name}_program, which part is moved from run_rc_command), and
> >>>>>>>>setting
> >>>>>>>>etcdir accordingly.
> >>>>>>>>
> >>>>>>>>The point is that some scripts (like rc.d/sshd) can be used  
> >>>>>>>>for base
> >>>>>>>>sshd as well as ports sshd, and makes possible to source
> >>>>>>>>${prefix}/etc/rc.conf.d/${name}.
> >>>>>>>>
> >>>>>>>>This patch also documents ${name}_program above run_rc_command
> >>>>>>>>(though
> >>>>>>>>it's actually used in load_rc_config).
> >>>>>>>
> >>>>>>>Is command always set?  I'm pretty sure it isn't so this may  
> >>>>>>>not be
> >>>>>>>entierly
> >>>>>>>safe.  If it's not set, should we try to guess prefix from $0?
> >>>>>>
> >>>>>>Somehow, command gets set to the right value, but you're  
> >>>>>>right, I'm
> >>>>>>missing a bit here.
> >>>>>
> >>>>>Hum, re-reading rc.subr, you were right, so I just did what you
> >>>>>supposed.
> >>>>
> >>>>Thinking about this a bit more, in the guessing frmo $0 case,  
> >>>>your proposed
> >>>>code:
> >>>>
> >>>>+		prefix=${0%/etc/rc.d/*}/
> >>>>
> >>>>won't work reliably when the user uses a relative path. I think  
> >>>>something
> >>>>like this would be better:
> >>>>
> >>>>		_tmp=`/bin/realpath $0`
> >>>>		prefix=${_tmp%/etc/rc.d/*}/
> >>>
> >>>Indeed, fixed.
> >>>
> >>>>>>>The other issue I see is that instead of:
> >>>>>>>
> >>>>>>>	if [ -f ${etcdir}/rc.conf.d/"$_command" ]; then
> >>>>>>>		debug "Sourcing ${etcdir}/rc.conf.d/${_command}"
> >>>>>>>		. ${etcdir}/rc.conf.d/"$_command"
> >>>>>>>	fi
> >>>>>>>
> >>>>>>>I think we should do:
> >>>>>>>
> >>>>>>>	if [ -f /etc/rc.conf.d/"$_command" ]; then
> >>>>>>>		debug "Sourcing /etc/rc.conf.d/${_command}"
> >>>>>>>		. /etc/rc.conf.d/"$_command"
> >>>>>>>	fi
> >>>>>>>	if [ "${etcdir}" != "/etc" -a -f ${etcdir}/
> >>>>>>>rc.conf.d/"$_command" ]; then
> >>>>>>>		debug "Sourcing ${etcdir}/rc.conf.d/${_command}"
> >>>>>>>		. ${etcdir}/rc.conf.d/"$_command"
> >>>>>>>	fi
> >>>>>>>
> >>>>>>>That preserves the old behavior while adding support for
> >>>>>>>${prefix}/etc/rc.conf.d.
> >>>>>>
> >>>>>>Fair enough, but I'd like to add a note saying that /etc/ 
> >>>>>>rc.conf.d/$
> >>>>>>{name} is deprecated for ${etcdir} != "/etc".
> >>>>
> >>>>The deprecation warning should not be printed in the case that $ 
> >>>>{etcdir}
> >>>>is /etc.  You should also avoid sourcing the file twice in the /etc
> >>>>case.  The easiest way to do that is probably to make the first  
> >>>>case
> >>>>contingent on ${etcdir} != /etc.
> >>>
> >>>Next time I'll test my changes (and sleep more).
> >>>
> >>>Did that too, and added a check to test if there's a
> >>>${etcdir}/rc.conf.d/${_command} file.
> >>
> >>Testing prefix=/ isn't sufficent since prefix could also be /usr.   
> >>You
> >>should check etcdir=/etc.
> >
> >True.
> >
> >>I don't think there's much point in the second test.  I don't like  
> >>silent
> >>ignoring of files, it's really hard to debug.  Instead, I'd source  
> >>the file
> >>in that case, but print a warning that two files exist.
> >
> >I thought there might be cases where you'd want different options  
> >in /etc/rc.conf.d/$name and $etcdir/rc.conf.d/$name. So keeping  
> >both made sense.
> >
> >>>Patch updated : http://people.freebsd.org/~flz/local/rc.d-sshd.diff
> >>>
> >>>BTW, I think that we should s/_command/_name/ in load_rc_config 
> >>>(), this
> >>>is a bit confusing.
> >>
> >>That sounds reasonable.
> >
> >Ok, will do then.
> >
> >I've merged latest rc.subr changes from NetBSD too, will post the  
> >diff with everything tomorrow in the (european) morning.
> 
> Here's new diff (merge from NetBSD + load_rc_config changes) :
> 
> http://people.freebsd.org/~flz/local/rc.d-merge-sshd.diff
> 
> I'm still unsure about what you proposed (warning when both /etc/ 
> rc.conf.d/${name} and ${etcdir}/rc.conf.d/${name}  exist). It would  
> be nice to have comments from somebody else.

I suggested the warning because it's hard to diagnose issues since
a change to /etc/rc.conf.d/xxx won't do anything if it's set in
${etcdir}/etc/rc.conf.d/xxx in that case and you suggested marking
/etc/rc.conf.d/ deprecated for scripts who's etcdir wasn't /etc.  If
it's not deprecated, the warning isn't needed.

Other than resolving that issue one way or another I'm happy with this
patch.  I'd suggest committing the NetBSD changes separately from local
changes where possible.

-- Brooks

-- 
Any statement of the form "X is the one, true Y" is FALSE.
PGP fingerprint 655D 519C 26A7 82E7 2529  9BF0 5D8E 8BE9 F238 1AD4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-rc/attachments/20060403/5f2fd0ba/attachment.pgp