Finding $pidfile from a conf file (Was: Re: conf/153460: devd(8) cannot be restarted correctly via /etc/rc.d script)

[Doug Barton]

On 04/21/2011 16:28, Jilles Tjoelker wrote:

>> 		line="/${line%%[\"\;]*}"
>
> The meaning of this line depends on the version of sh(1). The correct
> interpretation, used by sh in 9-current and most other shells, is to
> strip from the first double-quote or semicolon onwards. However, sh in
> older FreeBSD versions will strip from the first backslash, double-quote
> or semicolon onwards.
>
> If the 9-current behaviour is desired for all FreeBSD versions, use:
> 		line=/${line%%[\"\;]*}

I think it's incredibly unlikely that there would actually be a 
backslash in the text, and even if there was, it should be ok to strip 
from there. In the almost equally unlikely even that there is a space in 
the path name of the pid file, I think the quotes are safer, and 
future-proof.

> Perhaps spaces should be treated as a field terminator as well.

At least one conf file that I found spells "pid file" with a space, so I 
don't think that's safe.

>> if ! pidfile=`get_pidfile_from_conf "$1" "$2" 2>/dev/null`; then
>
> This kind of construction invokes the function in a subshell, which is
> less efficient. Passing the name of a variable to write the result into
> is more efficient.

Right, but I was trying to avoid having to use a global if at all 
possible. My concern being that what's likely to happen is that someone 
is going to use "pidfile" as the variable to pass in. I suppose what I 
could do however is change the function to set something ugly/unique 
like $_pidfile_from_conf and then have the example implementation look 
like this:

if get_pidfile_from_conf string file ; then
	pidfile="$_pidfile_from_conf"
else
	pidfile="some default"
fi

FWIW, this would be different from the way that almost all other rc.subr 
functions that set values work, they use the echo method overwhelmingly. 
Compare for example set_rcvar() which is used by almost all of the rc.d 
scripts. (OTOH, one of the reasons I recommend hard-coding the value 
instead is precisely because of this inefficiency, it adds about 1 full 
second to the boot time.)

I just did a quick test and the subshell version runs 10,000 times in 
about 5.2 seconds of wall clock time, which even to me seems efficient 
enough. :)  I can do the pass-by-reference version (which admittedly is 
about 4 times faster) though if you/others feel strongly that it's 
better to do it that way.

> It is advisable to set up the init script such that work like
> get_pidfile_from_conf is only done if the service is actually enabled.

Of course. I would imagine that people would use this in a start_precmd. 
That's certainly how I plan to do it in devd and named.

Thanks for the review!


Doug

-- 

	Nothin' ever doesn't change, but nothin' changes much.
			-- OK Go

	Breadth of IT experience, and depth of knowledge in the DNS.
	Yours for the right price.  :)  http://SupersetSolutions.com/