Date: Wed, 28 Feb 2007 19:27:06 +0100 From: Gabor Kovesdan <gabor@FreeBSD.org> To: Doug Barton <dougb@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: <45E5C97A.6060401@FreeBSD.org> In-Reply-To: <45E3701E.9000900@FreeBSD.org> References: <200702261857.l1QIvVMT051664@repoman.freebsd.org> <45E3701E.9000900@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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 > ... > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?45E5C97A.6060401>