Code review comment for lp:~oreilldf/wicd/resolvconf2

I suggest changing the last chunk of the patch as follows:

---8<---
@@ -518,7 +496,51 @@
             return
         if self.verbose: print cmd
         misc.Run(cmd)
-
+
+ def SetDNS(self, dns1=None, dns2=None, dns3=None,
+ dns_dom=None, search_dom=None):
+ """ Set the DNS of the system to the specified DNS servers.
+
+ Opens up resolv.conf and writes in the nameservers.
+
+ Keyword arguments:
+ dns1 -- IP address of DNS server 1
+ dns2 -- IP address of DNS server 2
+ dns3 -- IP address of DNS server 3
+ dns_dom -- DNS domain
+ search_dom -- DNS search domain
+
+ """
+ resolv_params = ""
+ if dns_dom:
+ resolv_params = ''.join([resolv_params, 'domain ', dns_dom, '\n'])
+ if search_dom:
+ resolv_params = ''.join([resolv_params, 'search ', search_dom,
+ '\n'])
+ valid_dns_list = []
+ for dns in [dns1, dns2, dns3]:
+ if dns:
+ if misc.IsValidIP(dns):
+ print 'Setting DNS : ' + dns
+ valid_dns_list.append(dns)
+ else:
+ print 'DNS IP %s is not a valid IP address, skipping' % dns
+
+ if self.resolvconf_cmd:
+ # Make sure we have at least one DNS in the list.
+ if valid_dns_list:
+ resolv_params += 'nameserver ' + ' '.join(valid_dns_list) + '\n'
+
+ print "running resolvconf"
+ p = misc.Run(' '.join([self.resolvconf_cmd, '-a', self.iface]),
+ include_stderr=True, return_obj=True)
+ p.communicate(input=resolv_params)[0]
+ else:
+ resolv = open("/etc/resolv.conf", "w")
+ resolv.write(resolv_params + "\n")
+ for dns in valid_dns_list:
+ resolv.write('nameserver %s\n' % dns)
+ resolv.close()

     def FlushRoutes(self):
         """ Flush network routes for this device. """
--->8---

Let's go step-by-step.
Here:

+ else:
+ print 'DNS IP is not a valid IP address, skipping'

It would be rather useful printing *which* DNS is not a valid IP address.

Also, later we have:

In the same file:

+ # Make sure we have more than just 'nameserver' in the list.
+ if len(valid_dns_list) > 1:
+ resolv_params += ' '.join(valid_dns_list) + '\n'
+
+ if self.resolvconf_cmd:
+ print "running resolvconf"
+ p = misc.Run(' '.join([self.resolvconf_cmd, '-a', self.iface]),
+ include_stderr=True, return_obj=True)
+ p.communicate(input=resolv_params)[0]
+ else:
+ resolv = open("/etc/resolv.conf", "w")
+ resolv.write(resolv_params + "\n")
+ resolv.close()

This is wrong. /etc/resolv.conf accepts maximum MAXNS (currently 3, see <resolv.h>) IPs listed after "nameserver", while resolvconf gracefully handles that by putting each IP on a separate line (i.e. "nameserver A B" used with resolvconf would actually give "nameserver A\nnameserver B" in /etc/resolv.conf).

Other than this, it seems fine to me, but it would obviously need some testing first by resolvconf users (I installed it just to implement it, I wasn't/am not using it)