cvs commit: ports/security/vpnc Makefile ports/security/vpnc/files vpnc.in vpnc.sh

[Gabor Kovesdan]

Doug Barton schrieb:
> Gabor Kovesdan wrote:
>   
>> gabor       2007-02-26 18:57:31 UTC
>>
>>   FreeBSD ports repository
>>
>>   Modified files:
>>     security/vpnc        Makefile 
>>   Added files:
>>     security/vpnc/files  vpnc.in 
>>   Log:
>>   - Update rc.d script
>>   - Use USE_RC_SUBR instead of direct patching
>>   - Bump PORTREVISION
>>   
>>   PR:             ports/107675  http://www.FreeBSD.org/cgi/query-pr.cgi?pr=107675
>>   Submitted by:   Dominic Fandrey <lon_kamikaze at gmx.de> (with fixes from maintainer)
>>   Approved by:    Christian Lackas <delta at lackas.net> (maintainer),
>>                   erwin (mentor, implicit)
>>   
>>   Revision  Changes    Path
>>   1.21      +4 -6      ports/security/vpnc/Makefile
>>   1.1       +95 -0     ports/security/vpnc/files/vpnc.in (new)
>>
>> http://www.FreeBSD.org/cgi/cvsweb.cgi/ports/security/vpnc/Makefile.diff?&r1=1.20&r2=1.21&f=h
>> http://www.FreeBSD.org/cgi/cvsweb.cgi/ports/security/vpnc/files/vpnc.in
>>     
>
> First off I'd like to say that I always appreciate when people convert
> their ports to the new rc.d framework, so please don't take any of
> these comments as critical. They are meant to be constructive,
> especially given that there are so many people involved in this one
> commit. :)
>
> The Makefile changes look good, but there are a few problems with the
> new rc.d script. If you have any questions don't hesitate to ask for
> clarification.
>
> 1. Please remove the FreeBSD KEYWORD. We have ignored that keyword for
> a long time now, and yet somehow it keeps creeping back in. You might
> also consider changing the REQUIRE to LOGIN, unless there is a
> compelling reason to have it where it is.
>
> 2. In your default variable assignments, it's not necessary (and
> probably not a good idea) to add empty assignments for _conf and
> _flags. Rather you should document those flags in the comments as you
> do the _conf variable.
>
> 3. In vpnc_start(), you don't want to use a bare 'if [ "$variable" ]'.
> That's not good style, and it opens you up to problems. I would write
> that section like this:
>
> if [ -z "$vpnc_conf" ]; then
> 	# No configuration files given, run unmanaged.
> 	$command $vpnc_flags
> 	return $?
> fi
>
> # A list of configurations is present. Connect managing
> ....
>
> 4. In both functions, you use the following style:
> for foo in $bar; (
> 	...
> )
>
> It's probably better to use:
> for foo in $bar; do
> 	...
> done
>
> That way you avoid the subshell, and any possible related problems.
>
> 5. In vpnc_start() you could simplify the code by doing:
>
> if ! $command $current $vpnc_flags; then
> 	status=$?
> 	echo "Running 'vpnc $current $vpnc_flags' failed."
> 	return $status
> fi
>
> # Move files to allow a clean shutdown
> ...
>
>
> 6. For simplicity, I'd make the same change in vpnc_stop() that I
> suggested in _start, namely to do:
>
> if [ ! -e "$vpnc_record" ]; then
> 	/bin/sleep 1
> 	# There's no record of connections, assume unmanaged shutdown.
> 	$command-disconnect
> 	return $?
> fi
>
> # A record of vpnc connections is present. Attempt a
> ...
>   
Hello,

thanks for pointing these out. Could you check the patch at 
http://gabor.t-hosting.hu/patches/security-vpnc.diff to make sure I got 
all your points correctly? While here, I changed the wrapping a bit to 
make it easier to read.
Christian, do you approve these changes suggested by Doug?

Regards,
Gabor