Date: Thu, 30 Mar 2006 00:48:24 +0100 From: Florent Thoumie <flz@xbsd.org> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: freebsd-rc@freebsd.org Subject: Re: rc.subr / rc.d/sshd patch for review Message-ID: <D1279151-8F6F-4D9A-A17C-63490E3BA9A8@xbsd.org> In-Reply-To: <9783E661-7B92-47ED-ABF3-EC1AC4369CE0@xbsd.org> References: <1143202549.16053.145.camel@mayday.esat.net> <20060324205627.GA18100@odin.ac.hmc.edu> <00E087F1-81E4-4580-A655-50F3DD8A471F@xbsd.org> <1143461191.4290.5.camel@mayday.esat.net> <20060327183745.GA19473@odin.ac.hmc.edu> <1143556715.65237.4.camel@mayday.esat.net> <20060328170842.GA16561@odin.ac.hmc.edu> <9783E661-7B92-47ED-ABF3-EC1AC4369CE0@xbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
--
Florent Thoumie
flz@FreeBSD.org
FreeBSD Committer
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D1279151-8F6F-4D9A-A17C-63490E3BA9A8>