Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 26 Feb 2007 15:41:18 -0800
From:      Doug Barton <dougb@FreeBSD.org>
To:        Gabor Kovesdan <gabor@FreeBSD.org>
Cc:        Dominic Fandrey <lon_kamikaze@gmx.de>, cvs-all@FreeBSD.org, ports-committers@FreeBSD.org, cvs-ports@FreeBSD.org, erwin@freebsd.org, Christian Lackas <delta@lackas.net>
Subject:   Re: cvs commit: ports/security/vpnc Makefile ports/security/vpnc/files vpnc.in vpnc.sh
Message-ID:  <45E3701E.9000900@FreeBSD.org>
In-Reply-To: <200702261857.l1QIvVMT051664@repoman.freebsd.org>
References:  <200702261857.l1QIvVMT051664@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help 
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@gmx.de> (with fixes from maintainer)
>   Approved by:    Christian Lackas <delta@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
...

(BTW, there is a type at "asume")


hope this helps,

Doug

-- 

    This .signature sanitized for your protection

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?45E3701E.9000900>