Merge lp:~oreilldf/wicd/resolvconf2 into lp:wicd/1.6

Proposed by Dan O'Reilly
Status: Merged
Approved by: Dan O'Reilly
Approved revision: 327
Merged at revision: not available
Proposed branch: lp:~oreilldf/wicd/resolvconf2
Merge into: lp:wicd/1.6
Diff against target: None lines
To merge this branch: bzr merge lp:~oreilldf/wicd/resolvconf2
Reviewer Review Type Date Requested Status
Dan O'Reilly Approve
David Paleino Approve
Review via email: mp+4504@code.launchpad.net

This proposal supersedes a proposal from 2009-03-13.

To post a comment you must log in.
Revision history for this message
Dan O'Reilly (oreilldf) wrote : Posted in a previous version of this proposal

Adds support for using resolvconf to set DNS settings instead of directly editing resolv.conf.

Revision history for this message
David Paleino (dpaleino) wrote : Posted in a previous version of this proposal
Download full text (3.3 KiB)

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).

O...

Read more...

review: Needs Fixing
Revision history for this message
David Paleino (dpaleino) wrote :

The only thing I noted is line 174 of the diff:

174 + for dns in (dns1, dns2, dns3):

you were using a [] before, IIRC. Any particular reason for this? :)

Other than that, seems fine to me, marking as Approved. In case any bugs arise, I'll surely discover them in Debian, patch them and re-submit upstream.

Thank you!
David

review: Approve
Revision history for this message
Dan O'Reilly (oreilldf) wrote :

Nope, I was actually just messing around with the "surround" plugin for vim and forgot that I had changed it to a tuple instead of a list. :)

Thanks for your help getting this delivered.

Revision history for this message
Dan O'Reilly (oreilldf) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'wicd/misc.py'
2--- wicd/misc.py 2009-03-09 03:39:41 +0000
3+++ wicd/misc.py 2009-03-09 04:07:39 +0000
4@@ -95,6 +95,10 @@
5 else:
6 err = None
7 fds = False
8+ if return_obj:
9+ std_in = PIPE
10+ else:
11+ std_in = None
12
13 # We need to make sure that the results of the command we run
14 # are in English, so we set up a temporary environment.
15@@ -105,8 +109,8 @@
16 tmpenv["LANG"] = __LANG
17
18 try:
19- f = Popen(cmd, shell=False, stdout=PIPE, stderr=err, close_fds=fds,
20- cwd='/', env=tmpenv)
21+ f = Popen(cmd, shell=False, stdout=PIPE, stdin=std_in, stderr=err,
22+ close_fds=fds, cwd='/', env=tmpenv)
23 except OSError, e:
24 print "Running command %s failed: %s" % (str(cmd), str(e))
25 return ""
26
27=== modified file 'wicd/networking.py'
28--- wicd/networking.py 2009-03-09 03:39:41 +0000
29+++ wicd/networking.py 2009-03-09 04:07:39 +0000
30@@ -427,7 +427,7 @@
31 return
32
33 @abortable
34- def set_dns_addresses(self):
35+ def set_dns_addresses(self, iface):
36 """ Set the DNS address(es).
37
38 If static DNS servers or global DNS servers are specified, set them.
39@@ -435,19 +435,19 @@
40
41 """
42 if self.network.get('use_global_dns'):
43- BACKEND.SetDNS(misc.Noneify(self.global_dns_1),
44- misc.Noneify(self.global_dns_2),
45- misc.Noneify(self.global_dns_3),
46- misc.Noneify(self.global_dns_dom),
47- misc.Noneify(self.global_search_dom))
48+ iface.SetDNS(misc.Noneify(self.global_dns_1),
49+ misc.Noneify(self.global_dns_2),
50+ misc.Noneify(self.global_dns_3),
51+ misc.Noneify(self.global_dns_dom),
52+ misc.Noneify(self.global_search_dom))
53 elif self.network.get('use_static_dns') and (self.network.get('dns1') or
54 self.network.get('dns2') or self.network.get('dns3')):
55 self.SetStatus('setting_static_dns')
56- BACKEND.SetDNS(self.network.get('dns1'),
57- self.network.get('dns2'),
58- self.network.get('dns3'),
59- self.network.get('dns_domain'),
60- self.network.get('search_domain'))
61+ iface.SetDNS(self.network.get('dns1'),
62+ self.network.get('dns2'),
63+ self.network.get('dns3'),
64+ self.network.get('dns_domain'),
65+ self.network.get('search_domain'))
66
67 @abortable
68 def release_dhcp_clients(self, iface):
69@@ -837,7 +837,7 @@
70 # Set up gateway, IP address, and DNS servers.
71 self.set_broadcast_address(wiface)
72 self.set_ip_address(wiface)
73- self.set_dns_addresses()
74+ self.set_dns_addresses(wiface)
75
76 # Run post-connection script.
77 self.run_global_scripts_if_needed(wpath.postconnectscripts)
78@@ -1025,7 +1025,7 @@
79 # Set gateway, IP adresses, and DNS servers.
80 self.set_broadcast_address(liface)
81 self.set_ip_address(liface)
82- self.set_dns_addresses()
83+ self.set_dns_addresses(liface)
84
85 # Run post-connection script.
86 self.run_global_scripts_if_needed(wpath.postconnectscripts)
87
88=== modified file 'wicd/wnettools.py'
89--- wicd/wnettools.py 2009-03-09 03:09:12 +0000
90+++ wicd/wnettools.py 2009-03-15 17:47:55 +0000
91@@ -74,7 +74,7 @@
92 blacklist_norm = ";`$!*|><&\\"
93 blank_trans = maketrans("", "")
94
95-__all__ = ["SetDNS", "GetDefaultGateway", "GetWiredInterfaces",
96+__all__ = ["GetDefaultGateway", "GetWiredInterfaces",
97 "GetWirelessInterfaces", "IsValidWpaSuppDriver"]
98
99 def _sanitize_string(string):
100@@ -89,33 +89,6 @@
101 else:
102 return string
103
104-def SetDNS(dns1=None, dns2=None, dns3=None, dns_dom=None, search_dom=None):
105- """ Set the DNS of the system to the specified DNS servers.
106-
107- Opens up resolv.conf and writes in the nameservers.
108-
109- Keyword arguments:
110- dns1 -- IP address of DNS server 1
111- dns2 -- IP address of DNS server 2
112- dns3 -- IP address of DNS server 3
113- dns_dom -- DNS domain
114- search_dom -- DNS search domain
115-
116- """
117- resolv = open("/etc/resolv.conf", "w")
118- if dns_dom:
119- resolv.write("domain %s\n" % dns_dom)
120- if search_dom:
121- resolv.write('search %s\n' % search_dom)
122- for dns in [dns1, dns2, dns3]:
123- if dns:
124- if misc.IsValidIP(dns):
125- print 'Setting DNS : ' + dns
126- resolv.write('nameserver ' + dns + '\n')
127- else:
128- print 'DNS IP is not a valid IP address, not writing to resolv.conf'
129- resolv.close()
130-
131 def GetDefaultGateway():
132 """ Attempts to determine the default gateway by parsing route -n. """
133 route_info = misc.Run("route -n")
134@@ -291,6 +264,11 @@
135 self.CheckWirelessTools()
136 self.CheckSudoApplications()
137 self.CheckRouteFlushTool()
138+ self.CheckResolvConf()
139+
140+ def CheckResolvConf(self):
141+ """ Checks for the existence of resolvconf."""
142+ self.resolvconf_cmd = self._find_program_path("resolvconf")
143
144 def CheckDHCP(self):
145 """ Check for the existence of valid DHCP clients.
146@@ -518,7 +496,50 @@
147 return
148 if self.verbose: print cmd
149 misc.Run(cmd)
150-
151+
152+ def SetDNS(self, dns1=None, dns2=None, dns3=None,
153+ dns_dom=None, search_dom=None):
154+ """ Set the DNS of the system to the specified DNS servers.
155+
156+ Opens up resolv.conf and writes in the nameservers.
157+
158+ Keyword arguments:
159+ dns1 -- IP address of DNS server 1
160+ dns2 -- IP address of DNS server 2
161+ dns3 -- IP address of DNS server 3
162+ dns_dom -- DNS domain
163+ search_dom -- DNS search domain
164+
165+ """
166+ if not self.iface: return False
167+ resolv_params = ""
168+ if dns_dom:
169+ resolv_params += 'domain %s\n' % dns_dom
170+ if search_dom:
171+ resolv_params += 'search %s\n' % search_dom
172+
173+ valid_dns_list = []
174+ for dns in (dns1, dns2, dns3):
175+ if dns:
176+ if misc.IsValidIP(dns):
177+ if self.verbose:
178+ print 'Setting DNS : ' + dns
179+ valid_dns_list.append("nameserver %s\n" % dns)
180+ else:
181+ print 'DNS IP %s is not a valid IP address, skipping' % dns
182+
183+ if valid_dns_list:
184+ resolv_params += ''.join(valid_dns_list)
185+
186+ if self.resolvconf_cmd:
187+ cmd = [self.resolvconf_cmd, '-a', self.iface]
188+ if self.verbose: print cmd
189+ p = misc.Run(cmd, include_stderr=True, return_obj=True)
190+ p.communicate(input=resolv_params)
191+ else:
192+ resolv = open("/etc/resolv.conf", "w")
193+ resolv.write(resolv_params + "\n")
194+ resolv.close()
195
196 def FlushRoutes(self):
197 """ Flush network routes for this device. """

Subscribers

People subscribed via source and target branches

to status/vote changes: