Merge lp:~smoser/ubuntu/oneiric/isc-dhcp/lp857524 into lp:ubuntu/oneiric/isc-dhcp

[Scott Moser]

fix updating of /etc/resolv.conf for read-only /etc/

This makes the following changes:
 * instead of writing to a temp file in /etc/, write to a temp file in
   same directory as resolv.conf (which might be different if
   /etc/resolv.conf is a link)
 * replace the multiple '>>' with a single '>' and braces. This means
   the temp file will be opened for write once rather than append many times.
 * use 'grep -i' rather than 'sed' to get the old nameserver entries from
   existing resolv.conf. This is how the linux.udeb does it, and I find it more
   clean.
 * changes 'wait_for_rw' to take a file argument rather than hard coding /etc
   and then actually tests that that file is writable by opening it for append

[Marc Deslauriers (mdeslaur)]

Won't this result in potential problems while the /etc/resolv.conf file is in the process of being written to? Having the mv from the temp file results in it being atomic...

[Scott Moser (smoser)]

I suppose, yes. There is the potential for something to race on that file.
The old code path (and with a move) would still potentially have a race conditoin when it tries to read /etc/resolv.conf and get the old name servers, and then uses those for updating. Something could edit in between.

All that said, this should be *really* fast.
neither the DHCPv6 or DCHPv4 paths do any IO during the writing of that file.
Its all just shell builtins and variable operations.

You're correct, though, there is the potential for race. Should this be NAK'd because of that?

[Colin Watson (cjwatson)]

Nak - resolving the symlink is a much better approach.

[Colin Watson (cjwatson)]

review: Disapprove

[lp:~smoser/ubuntu/oneiric/isc-dhcp/lp857524]

38. By Scott Moser on 2011-10-24

make the population of resolv.conf atomic with a move

This changes the previous commit by:
 * re-adding the use of chmod and chown --reference
 * using readlink -f to create a temp file to get the full path
   to /etc/resolv.conf (in the case it is a symlink)
 * creates a temp file in that directory and populates the final
   /etc/resolv.conf with a move.

39. By Scott Moser on 2011-10-24

remove unnecessary '{}' in chown/chmod call

[Scott Moser (smoser)]

I've re-written with 'mv' and a temp file so the population of /etc/resolv.conf is now atomic with respect to this process.

[Colin Watson (cjwatson)]

This looks OK to me now. Thanks!

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2011-09-23 11:26:29 +0000
+++ debian/changelog	2011-10-24 16:08:30 +0000
@@ -1,3 +1,9 @@
+isc-dhcp (4.1.1-P1-17ubuntu11) UNRELEASED; urgency=low
+
+  * debian/dhclient-script.linux: fix for read-only /etc (LP: #857524)
+
+ -- Scott Moser <[e-mail]>  Fri, 23 Sep 2011 12:07:24 -0400
+
 isc-dhcp (4.1.1-P1-17ubuntu10) oneiric; urgency=low
 
   * make sure writing of /etc/resolv.conf actually waits until the file is 

=== modified file 'debian/dhclient-script.linux'
--- debian/dhclient-script.linux	2011-09-23 11:26:29 +0000
+++ debian/dhclient-script.linux	2011-10-24 16:08:30 +0000
@@ -12,8 +12,9 @@
 
 # The alias handling in here probably still sucks. -mdz
 
-# wait for /etc/resolv.conf to become writable
+# wait for given file to be writable
 wait_for_rw() {
+    local file=$1
     # Find out whether we are going to mount / rw
     exec 9>&0 </etc/fstab
     rootmode=rw
@@ -27,9 +28,9 @@
      done
      exec 0>&9 9>&-
  
-    # Wait for /etc/resolv.conf to become writable
+    # Wait for $file to become writable
     if [ "$rootmode" = "rw" ]; then
-        while ! { : >> /etc/resolv.conf; } 2>/dev/null; do
+        while ! { : >> "$file"; } 2>/dev/null; do
             sleep 0.1
         done
     fi
@@ -37,17 +38,23 @@
 
 # update /etc/resolv.conf based on received values
 make_resolv_conf() {
-    local new_resolv_conf
+    local old_ns="" resolv_conf="" new_resolv_conf=""
+    [ -e /etc/resolv.conf ] && old_ns=$(grep -i '^nameserver' /etc/resolv.conf)
+
+    resolv_conf=$(readlink -f "/etc/resolv.conf" 2>/dev/null) ||
+        resolv_conf="/etc/resolv.conf"
+
+    # write to temp file in same directory as target
+    new_resolv_conf="${resolv_conf}.dhclient-new.$$"
 
     # DHCPv4
     if [ -n "$new_domain_search" ] || [ -n "$new_domain_name" ] ||
        [ -n "$new_domain_name_servers" ]; then
-        wait_for_rw
-        new_resolv_conf=/etc/resolv.conf.dhclient-new
-        rm -f $new_resolv_conf
+        wait_for_rw "$new_resolv_conf"
 
+        {
         if [ -n "$new_domain_name" ]; then
-            echo domain ${new_domain_name%% *} >>$new_resolv_conf
+            echo domain ${new_domain_name%% *}
         fi
 
         if [ -n "$new_domain_search" ]; then
@@ -63,44 +70,44 @@
                     new_domain_search="$new_domain_name $new_domain_search"
                 fi
             fi
-            echo "search ${new_domain_search}" >> $new_resolv_conf
+            echo "search ${new_domain_search}"
         elif [ -n "$new_domain_name" ]; then
-            echo "search ${new_domain_name}" >> $new_resolv_conf
+            echo "search ${new_domain_name}"
         fi
 
         if [ -n "$new_domain_name_servers" ]; then
             for nameserver in $new_domain_name_servers; do
-                echo nameserver $nameserver >>$new_resolv_conf
+                echo nameserver $nameserver
             done
-        else # keep 'old' nameservers
-            sed -n /^\w*[Nn][Aa][Mm][Ee][Ss][Ee][Rr][Vv][Ee][Rr]/p /etc/resolv.conf >>$new_resolv_conf
+        elif [ -n "$old_ns" ]; then # keep 'old' nameservers
+            echo "$old_ns"
         fi
-
-        chown --reference=/etc/resolv.conf $new_resolv_conf
-        chmod --reference=/etc/resolv.conf $new_resolv_conf
-        mv -f $new_resolv_conf /etc/resolv.conf
+        } > "$new_resolv_conf"
     # DHCPv6
     elif [ -n "$new_dhcp6_domain_search" ] || [ -n "$new_dhcp6_name_servers" ]; then
-        wait_for_rw
-        new_resolv_conf=/etc/resolv.conf.dhclient-new
-        rm -f $new_resolv_conf
+        wait_for_rw "$new_resolv_conf"
 
+        {
         if [ -n "$new_dhcp6_domain_search" ]; then
-            echo "search ${new_dhcp6_domain_search}" >> $new_resolv_conf
+            echo "search ${new_dhcp6_domain_search}"
         fi
 
         if [ -n "$new_dhcp6_name_servers" ]; then
             for nameserver in $new_dhcp6_name_servers; do
-                echo nameserver $nameserver >>$new_resolv_conf
+                echo nameserver $nameserver
             done
-        else # keep 'old' nameservers
-            sed -n /^\w*[Nn][Aa][Mm][Ee][Ss][Ee][Rr][Vv][Ee][Rr]/p /etc/resolv.conf >>$new_resolv_conf
+        elif [ -n "$old_ns" ]; then # keep 'old' nameservers
+            echo "$old_ns"
         fi
-
-        chown --reference=/etc/resolv.conf $new_resolv_conf
-        chmod --reference=/etc/resolv.conf $new_resolv_conf
-        mv -f $new_resolv_conf /etc/resolv.conf
+        } > "$new_resolv_conf"
     fi
+
+    [ -e "$resolv_conf" ] &&
+        chmod --reference="$resolv_conf" "$new_resolv_conf" &&
+        chown --reference="$resolv_conf" "$new_resolv_conf"
+    
+    mv "$new_resolv_conf" "$resolv_conf" ||
+        { rm -f "$new_resolv_conf"; return 1; }
 }
 
 # run given script