From owner-cvs-all@FreeBSD.ORG Wed Feb 28 18:27:45 2007 Return-Path: X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 9C42116A400; Wed, 28 Feb 2007 18:27:45 +0000 (UTC) (envelope-from gabor@FreeBSD.org) Received: from server.t-hosting.hu (server.t-hosting.hu [217.20.133.7]) by mx1.freebsd.org (Postfix) with ESMTP id E85F313C4A3; Wed, 28 Feb 2007 18:27:40 +0000 (UTC) (envelope-from gabor@FreeBSD.org) Received: from localhost (localhost [127.0.0.1]) by server.t-hosting.hu (Postfix) with ESMTP id 2B45C9EFB86; Wed, 28 Feb 2007 19:27:40 +0100 (CET) X-Virus-Scanned: amavisd-new at t-hosting.hu Received: from server.t-hosting.hu ([127.0.0.1]) by localhost (server.t-hosting.hu [127.0.0.1]) (amavisd-new, port 10024) with LMTP id l-oisODywOQD; Wed, 28 Feb 2007 19:27:12 +0100 (CET) Received: from [192.168.2.186] (catv-50635cb6.catv.broadband.hu [80.99.92.182]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by server.t-hosting.hu (Postfix) with ESMTP id 2B2149EFB49; Wed, 28 Feb 2007 19:27:12 +0100 (CET) Message-ID: <45E5C97A.6060401@FreeBSD.org> Date: Wed, 28 Feb 2007 19:27:06 +0100 From: Gabor Kovesdan User-Agent: Thunderbird 1.5.0.9 (Windows/20061207) MIME-Version: 1.0 To: Doug Barton References: <200702261857.l1QIvVMT051664@repoman.freebsd.org> <45E3701E.9000900@FreeBSD.org> In-Reply-To: <45E3701E.9000900@FreeBSD.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Dominic Fandrey , cvs-all@FreeBSD.org, ports-committers@FreeBSD.org, cvs-ports@FreeBSD.org, erwin@freebsd.org, Christian Lackas Subject: Re: cvs commit: ports/security/vpnc Makefile ports/security/vpnc/files vpnc.in vpnc.sh X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Feb 2007 18:27:45 -0000 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 (with fixes from maintainer) >> Approved by: Christian Lackas (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