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

[Florent Thoumie]

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.

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.

-- 
Florent Thoumie
flz at FreeBSD.org
FreeBSD Committer