Giuseppe Vallarelli has posted comments on this change. Change subject: netconf: Add dhcp support for iproute2 configurator ......................................................................
Patch Set 4: Code-Review-1 (3 comments) Minor comments pretty much looks okay to me. Thanks Mark. .................................................... File vdsm/netconf/iproute2.py Line 197: t = threading.Thread(target=self._dhclient, name='vdsm-dhclient-%s' Line 198: % self.iface) Line 199: t.daemon = True Line 200: t.start() Line 201: else: For the moment I only see async usage, is it useful to have also the sync usage ? I would rather not add code that we're not going to use. Line 202: rc, out, err = self._dhclient() Line 203: return rc Line 204: Line 205: def shutdown(self): Line 205: def shutdown(self): Line 206: try: Line 207: pid = int(open(self.pidFile).readline().strip()) Line 208: except IOError as e: Line 209: if e.errno == os.errno.ENOENT: Hi Mark I think we can remove a little bit of duplication perhaps with an helper function I'm talking about the except block If e.errno == something: pass else: raise What do you think? Line 210: pass Line 211: else: Line 212: raise Line 213: else: Line 250: def ifup(self, iface): Line 251: ipwrapper.linkSet(['dev', iface.name, 'up']) Line 252: _, _, _, bootproto, async = iface.getIpConfig() Line 253: if bootproto == 'dhcp': Line 254: dhclient = DhcpClient(iface.name) I usually like to have dependencies explicit i.e. passing DhcpClient to the ConfigApplier constructor or in the methods which create instances of it. In this way it's also easier to test. Line 255: dhclient.start(async) Line 256: Line 257: def ifdown(self, iface): Line 258: ipwrapper.linkSet(['dev', iface.name, 'down']) -- To view, visit http://gerrit.ovirt.org/15492 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iea88e8693e47fa51edb89c33344332c88c5c964d Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: Mark Wu <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
