Date: Tue, 28 Mar 2006 18:30:22 +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: <9783E661-7B92-47ED-ABF3-EC1AC4369CE0@xbsd.org> In-Reply-To: <20060328170842.GA16561@odin.ac.hmc.edu> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
--
Florent Thoumie
flz@FreeBSD.org
FreeBSD Committer
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9783E661-7B92-47ED-ABF3-EC1AC4369CE0>