Change in vdsm[master]: netconf: Add dhcp support for iproute2 configurator

asegurap at redhat.com asegurap at redhat.com
Tue Jul 2 22:41:20 UTC 2013


Antoni Segura Puimedon has posted comments on this change.

Change subject: netconf: Add dhcp support for iproute2 configurator
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(4 inline comments)

Very cool addition. Thanks! Just a few minor comments.

....................................................
File vdsm/netconf/iproute2.py
Line 162: class DhcpClient():
Line 163:     PID_FILE = '/var/run/dhclient-%s.pid'
Line 164:     LEASE_DIR = '/var/lib/dhclient/'
Line 165:     LEASE_FILE = LEASE_DIR + 'dhclient-%s.lease'
Line 166:     DHCLIENT = str(utils.CommandPath('dhclient', '/sbin/dhclient'))
Instead of using str here, you should just use DHCLIENT.cmd on line 176.
Line 167: 
Line 168:     def __init__(self, iface):
Line 169:         self.iface = iface
Line 170:         self.pidFile = self.PID_FILE % self.iface


Line 172:             utils.execCmd(['mkdir', self.LEASE_DIR])
Line 173:         self.leaseFile = (self.LEASE_FILE % self.iface)
Line 174: 
Line 175:     def _dhclient(self):
Line 176:         rc, out, err = utils.execCmd([self.DHCLIENT, '-1', '-pf',
Since I already request a change to this line in the comment above, I'd prefer to have a from utils import execCmd.
Line 177:                                       self.pidFile, '-lf', self.leaseFile,
Line 178:                                       self.iface])
Line 179:         return rc, out, err
Line 180: 


Line 189:             return rc
Line 190: 
Line 191:     def shutdown(self):
Line 192:         try:
Line 193:             pid = int(file(self.pidFile).readline().strip())
'file' is deprecated, 'open' should be used instead.
Line 194:         except IOError as e:
Line 195:             if e.errno == os.errno.ENOENT:
Line 196:                 pass
Line 197:             else:


Line 197:             else:
Line 198:                 raise
Line 199:         else:
Line 200:             try:
Line 201:                 os.kill(pid, signal.SIGKILL)
I wonder if maybe we should use SIGTERM to let dhclient gracefully stop.
Line 202:             except OSError as e:
Line 203:                 if e.errno == os.errno.ESRCH:
Line 204:                     pass
Line 205:                 else:


-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list