Re: [OpenWrt-Devel] [OpenWrt-Commits] r31561 - in packages/net/xl2tpd: . files patches

2012-05-05 Thread David Woodhouse 
On Fri, 2012-05-04 at 23:29 -0600, Philip Prindeville wrote:
 I'd combine all of these as:
 
 local device server username password ...

Hm. I was just trying to copy local style, which in ppp.sh had each one
with its usage. Which makes slightly more sense to me; it makes it
simpler to add and remove options.

  +   [ $peerdns -eq 1 ]  {
  +   peerdns=usepeerdns
  +   } || {
  +   peerdns=
  +   add_dns $config $dns
  +   }
 
 Not clear how this is better than:
 
 if [ $peerdns -eq 1 ]; then
 peerdns=userpeerdns
 else
 peerdns=
 add_dns $config $dns
 fi

Not sure, but again that's what's in ppp.sh. Perhaps something to do
with shell compatibility, but I don't know which shells couldn't cope
with your version (which is how I'd write it myself).


 I'd have done:
 
 cat __EOF__  ${optfile}
 ${keepalive:+lcp-echo-interval $interval lcp-echo-failure ${keepalive%%[, ]*}}
 ...
 __EOF__

That was my first thought too. I can't remember now why I didn't do it
that way. Perhaps I thought that some of those individual echo commands
might be conditional. But yeah, cat EOF would be cleaner.

 
  +
  +   xl2tpd-control remove l2tp-${config}
  +   # Wait and ensure pppd has died.
  +   while [ -d /sys/class/net/l2tp-${config} ]; do
  +   sleep 1
  +   done
 
 If there's a bug, this might never exit...  Maybe add a counter?

Makes sense.

 Use of quotes around ${optfile} is inconsistent.

Hm, true. I think they're fairly pointless quotes, but I should at least
be consistent about it.


}
 
 I'd try to match the current indent style.

This is a patch from elsewhere; not a local one. It's from the Fedora
package, and also been pushed upstream I believe. So I'd rather not mess
with it (except to remove the modprobe invocation).

-- 
dwmw2


smime.p7s
Description: S/MIME cryptographic signature
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [OpenWrt-Commits] r31561 - in packages/net/xl2tpd: . files patches

2012-05-04 Thread Philip Prindeville 
A little bit after the fact... I missed the original posting, but here are some 
comments.

On 5/2/12 1:28 PM, openwrt-comm...@openwrt.org wrote:
 Author: juhosg
 Date: 2012-05-02 21:28:10 +0200 (Wed, 02 May 2012)
 New Revision: 31561
 
 Added:
packages/net/xl2tpd/files/l2tp.sh
packages/net/xl2tpd/patches/120-kernel-mode-l2tp.patch
packages/net/xl2tpd/patches/130-no-kill-ipparam.patch
 Modified:
packages/net/xl2tpd/Makefile
packages/net/xl2tpd/files/xl2tpd.conf
 Log:
 Update and fix x2ltpd, add connect script
 
 Remove unwanted services from default configuration
 Ship xl2tpd-config
 Use kernel mode L2TP
 Don't scribble on ipparam
 
 Signed-off-by: David Woodhouse david.woodho...@intel.com
 
 Modified: packages/net/xl2tpd/Makefile
 ===
 --- packages/net/xl2tpd/Makefile  2012-05-02 19:28:09 UTC (rev 31560)
 +++ packages/net/xl2tpd/Makefile  2012-05-02 19:28:10 UTC (rev 31561)
 @@ -8,12 +8,14 @@
  include $(TOPDIR)/rules.mk
  
  PKG_NAME:=xl2tpd
 -PKG_VERSION:=1.3.0
 +PKG_VERSION:=1.3.1
  PKG_RELEASE:=1
  
  PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz
 -PKG_SOURCE_URL:=ftp://ftp.xelerance.com/xl2tpd/
 -PKG_MD5SUM:=28264284552c442b24cf421755a2bb48
 +# Host seems to be down.
 +#PKG_SOURCE_URL:=ftp://ftp.xelerance.com/xl2tpd/
 +PKG_MD5SUM:=cf61576fef5c2d6c68279a408ec1f0d5
 +PKG_SOURCE_URL:=http://pkgs.fedoraproject.org/lookaside/pkgs/xl2tpd/xl2tpd-$(PKG_VERSION).tar.gz/$(PKG_MD5SUM)/
  
  PKG_INSTALL:=1
  
 @@ -25,6 +27,7 @@
TITLE:=An L2TP (Layer 2 Tunneling Protocol) daemon
URL:=http://www.xelerance.com/software/xl2tpd/
SUBMENU:=VPN
 +  DEPENDS:=+ppp-mod-pppol2tp +ip +resolveip
  endef
  
  define Package/xl2tpd/description
 @@ -46,6 +49,7 @@
  define Package/xl2tpd/install
   $(INSTALL_DIR) $(1)/usr/sbin
   $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/local/sbin/xl2tpd $(1)/usr/sbin/
 + $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/local/sbin/xl2tpd-control 
 $(1)/usr/sbin/
  
   $(INSTALL_DIR) $(1)/etc/init.d
   $(INSTALL_BIN) ./files/xl2tpd.init $(1)/etc/init.d/xl2tpd
 @@ -56,6 +60,9 @@
  
   $(INSTALL_DIR) $(1)/etc/ppp
   $(INSTALL_DATA) ./files/options.xl2tpd $(1)/etc/ppp/
 +
 + $(INSTALL_DIR) $(1)/lib/network
 + $(INSTALL_DATA) ./files/l2tp.sh $(1)/lib/network
  endef
  
  $(eval $(call BuildPackage,xl2tpd))
 
 Added: packages/net/xl2tpd/files/l2tp.sh
 ===
 --- packages/net/xl2tpd/files/l2tp.sh (rev 0)
 +++ packages/net/xl2tpd/files/l2tp.sh 2012-05-02 19:28:10 UTC (rev 31561)
 @@ -0,0 +1,124 @@
 +find_route() {
 + ip route get $1 | sed -e 's/ /\n/g' | \
 +sed -ne '1p;/via/{N;p};/dev/{N;p};/src/{N;p};/mtu/{N;p}'
 +}
 +
 +scan_l2tp() {
 + config_set $1 device l2tp-$1
 +}
 +
 +stop_interface_l2tp() {
 + local config=$1
 + local lock=/var/lock/l2tp-${config}
 + local optfile=/tmp/l2tp/options.${config}
 + local l2tpcontrol=/var/run/xl2tpd/l2tp-control
 +
 + lock $lock
 +
 + [ -p ${l2tpcontrol} ]  echo r l2tp-${config}  ${l2tpcontrol}
 + rm -f ${optfile}
 +
 + for ip in $(uci_get_state network $1 serv_addrs); do
 + ip route del $ip 2/dev/null
 + done
 +
 + lock -u $lock
 +}
 +
 +setup_interface_l2tp() {
 + local config=$2
 + local lock=/var/lock/l2tp-${config}
 + local optfile=/tmp/l2tp/options.${config}
 +
 + lock $lock
 +
 + if [ ! -p /var/run/xl2tpd/l2tp-control ]; then
 + /etc/init.d/xl2tpd start
 + fi
 + 
 + local device

I'd combine all of these as:

local device server username password ...


 + config_get device $config device l2tp-$config
 +
 + local server
 + config_get server $config server
 +
 + local username
 + config_get username $config username
 +
 + local password
 + config_get password $config password
 +
 + local keepalive
 + config_get keepalive $config keepalive
 +
 + local pppd_options
 + config_get pppd_options $config pppd_options
 +
 + local defaultroute
 + config_get_bool defaultroute $config defaultroute 1
 + [ $defaultroute -eq 1 ]  \
 + defaultroute=defaultroute replacedefaultroute || 
 defaultroute=nodefaultroute
 +
 + local interval=${keepalive##*[, ]}
 + [ $interval != $keepalive ] || interval=5
 +
 + local dns
 + config_get dns $config dns
 +
 + local has_dns=0
 + local peer_default=1
 + [ -n $dns ]  {
 + has_dns=1
 + peer_default=0
 + }
 +
 + local peerdns
 + config_get_bool peerdns $config peerdns $peer_default
 +
 + [ $peerdns -eq 1 ]  {
 + peerdns=usepeerdns
 + } || {
 + peerdns=
 + add_dns $config $dns
 + }

Not clear how this is better than:

if [ $peerdns -eq 1 ]; then
peerdns=userpeerdns
else
peerdns=
add_dns $config $dns
fi


 +
 + local ipv6
 +