Merge lp:~smoser/ubuntu/oneiric/ifupdown/lp838968 into lp:ubuntu/oneiric/ifupdown

[Scott Moser]

  * invoke dhclient3 with '-1', meaning it should exit failure if it does
    not receive a response in 60 seconds. (LP: #838968)
  * fix the broken static-network-up-emitted event. This now keeps
    state of which interfaces have been brought up by marker files
    in /run/network named ifup.IFACE and a directory named
    static-network-up-emitted indicating static-network-up has been
    emitted.

[Steve Langasek (vorlon)]

I don't think the changes to the interface of the all_interfaces_up() and get_auto_interfaces() functions are an improvement. Why move the interface list to an argument to all_interfaces_up? Why returning the interface list from get_auto_interfaces() in a global variable instead of on stdout? Neither of these changes are necessary for fixing the bug, and I think they both make the code harder to read.

+# touch our own "marker" indicating that this interface has been brought up.
+date > "${MARK_DEV_PREFIX}$IFACE"
+

Makes sense to avoid use of touch, which is in /usr/bin; but we don't exactly need the datestamp. Better to use :> "${MARK_DEV_PREFIX}$IFACE" and avoid a needless fork.

+ # if no interfaces were passed in, then "all [given] were up"
+ [ $# -eq 0 ] && return 0

Redundant check. If $# -eq 0, the for loop immediately below is a no-op and you'll get the return 0 anyway; so better to leave this out.

+mkdir -p "${MARK_DEV_PREFIX%/*}" "${MARK_STATIC_NETWORK_EMITTED%/*}"

This call is always unnecessary. The /run/network directory will always be created for us by ifupdown, and both of these variables should always point at /run/network. Please drop this.

review: Needs Fixing

[Scott Moser (smoser)]

On Fri, 2 Sep 2011, Steve Langasek wrote:

> Review: Needs Fixing
> I don't think the changes to the interface of the all_interfaces_up()
> and get_auto_interfaces() functions are an improvement.
> Why move the interface list to an argument to all_interfaces_up?

I thought it made the function stand on its own a little better. It was
then easier to test for an arbitrary list of inputs (ie, adding eth9 to
the params should get me a "no").

It also means that all_interfaces_up has almost no chance of returning
failure due to something else failing. Ie, if it does the call to
'get_auto_interfaces' and *that* returns false, what should it return ?

> Why returning the interface list from get_auto_interfaces() in a global
> variable instead of on stdout?

It saves a fork.

> Neither of these changes are necessary for fixing the
> bug, and I think they both make the code harder to read.

I wrote the original, and I felt these were improvements.

> +# touch our own "marker" indicating that this interface has been brought up.
> +date > "${MARK_DEV_PREFIX}$IFACE"
> +
>
> Makes sense to avoid use of touch, which is in /usr/bin; but we don't
> exactly need the datestamp. Better to use :> "${MARK_DEV_PREFIX}$IFACE"
> and avoid a needless fork.

I'm willing to save the fork there. I found the date stamp nice to look
at when debugging.

> + # if no interfaces were passed in, then "all [given] were up"
> + [ $# -eq 0 ] && return 0
>
> Redundant check. If $# -eq 0, the for loop immediately below is a no-op
> and you'll get the return 0 anyway; so better to leave this out.

I knew it was redundant, but I wanted to make that obvious to someone
looking at the code. surely '[ $# -eq 0 ]' is abount as fast as anything
can be in shell.

> +mkdir -p "${MARK_DEV_PREFIX%/*}" "${MARK_STATIC_NETWORK_EMITTED%/*}"
> This call is always unnecessary. The /run/network directory will always
> be created for us by ifupdown, and both of these variables should always
> point at /run/network. Please drop this.

I wanted to ensure that if someone changed the value of
MARK_STATIC_NETWORK_EMITTED from a subdir of '/var/run/' (say, to
'/var/run/network/event-locks' or something) that this would be the only
change needed. After that, adding the mkdir of MARK_DEV_PREFIX comes at
really no cost.

I'm willing to save the fork.

[lp:~smoser/ubuntu/oneiric/ifupdown/lp838968]

64. By Scott Moser on 2011-09-02

address slangasek's objections to code restructuring.

 * remove the 'mkdir', parent dir must now exist.
   (it will in the case of /run/network)
 * do not use global _RET, but write list of interfaces
   to stdout in get_auto_interfaces.

[Steve Langasek (vorlon)]

Looks good to me now. Thanks for your patience :)

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2011-08-22 10:30:48 +0000
+++ debian/changelog	2011-09-02 19:24:24 +0000
@@ -1,3 +1,15 @@
+ifupdown (0.7~alpha5.1ubuntu4) UNRELEASED; urgency=low
+
+  * invoke dhclient3 with '-1', meaning it should exit failure if it does
+    not receive a response in 60 seconds. (LP: #838968)
+  * fix the broken static-network-up-emitted event. This now keeps
+    state of which interfaces have been brought up by marker files
+    in /run/network named ifup.IFACE and a directory named 
+    static-network-up-emitted indicating static-network-up has been
+    emitted.
+
+ -- Scott Moser <[e-mail]>  Thu, 01 Sep 2011 19:55:55 -0400
+
 ifupdown (0.7~alpha5.1ubuntu3) oneiric; urgency=low
 
   * Document what events are emitted properly. (LP: #819928)

=== modified file 'debian/ifupdown.upstart.if-up'
--- debian/ifupdown.upstart.if-up	2011-08-10 17:01:24 +0000
+++ debian/ifupdown.upstart.if-up	2011-09-02 19:24:24 +0000
@@ -1,4 +1,6 @@
 #!/bin/sh
+MARK_DEV_PREFIX="/run/network/ifup."
+MARK_STATIC_NETWORK_EMITTED="/run/network/static-network-up-emitted"
 
 set -e
 
@@ -13,6 +15,7 @@
 
 get_auto_interfaces() {
 	# write to stdout a list of interfaces configured for 'auto'
+	# in /etc/network/interfaces
 	local line="" found=" " iface
 	while read line; do
 		set -- ${line%%#*}
@@ -25,23 +28,21 @@
 	echo "$@"
 }
 
-all_interfaces_up () {
-	local uplist="" up="" iface="" oifs="$IFS" phys virt
-	if [ -f /var/run/network/ifstate ] ; then
-		IFS="="
-		while read phys virt; do
-			uplist="${uplist}|${phys}"
-		done < /var/run/network/ifstate
-		IFS="$oifs"
-	fi
-	uplist="|${uplist#|}|"
-	# Check to see if all auto interfaces are up now
+all_interfaces_up() {
+	# return true if all interfaces listed in /etc/network/interfaces as 'auto'
+	# are up.  if no interfaces are found there, then "all [given] were up"
+	local prefix="$1" iface=""
 	for iface in $(get_auto_interfaces); do
-		[ "${uplist#*|${iface}|}" != "${uplist}"  ] || return 1
+		# if cur interface does is not up, then all have not been brought up
+		[ -f "${prefix}${iface}" ] || return 1
 	done
 	return 0
 }
 
-if all_interfaces_up && mkdir /var/run/network/static-network-up-emitted 2>/dev/null; then
+# touch our own "marker" indicating that this interface has been brought up.
+: > "${MARK_DEV_PREFIX}$IFACE"
+
+if all_interfaces_up "${MARK_DEV_PREFIX}" &&
+	mkdir "${MARK_STATIC_NETWORK_EMITTED}" 2>/dev/null; then
 	initctl emit --no-wait static-network-up
 fi

=== modified file 'ifupdown.nw'
--- ifupdown.nw	2011-08-09 14:41:00 +0000
+++ ifupdown.nw	2011-09-02 19:24:24 +0000
@@ -4423,7 +4423,7 @@
 
   up
     [[ip link set dev %iface% address %hwaddress%]]
-    dhclient3 [[-e IF_METRIC=%metric%]] -pf /var/run/dhclient.%iface%.pid -lf /var/lib/dhcp3/dhclient.%iface%.leases %iface% \
+    dhclient3 [[-e IF_METRIC=%metric%]] -pf /var/run/dhclient.%iface%.pid -lf /var/lib/dhcp3/dhclient.%iface%.leases -1 %iface% \
         if (execable("/sbin/dhclient3"))
     dhclient [[-e IF_METRIC=%metric%]] -v -pf /var/run/dhclient.%iface%.pid -lf /var/lib/dhcp/dhclient.%iface%.leases %iface% \
         elsif (execable("/sbin/dhclient"))