Change in vdsm[master]: Move load_needed_modules to vdsm-tool

danken at redhat.com danken at redhat.com
Fri Aug 31 09:32:05 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: Move load_needed_modules to vdsm-tool
......................................................................


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

(4 inline comments)

....................................................
File vdsm-tool/load_needed_modules.py.in
Line 27: 
Line 28: EX_MODPROBE = '@MODPROBE_PATH@'
Line 29: 
Line 30: BOND = ['bond0', 'bond1', 'bond2', 'bond3', 'bond4']
Line 31: DEVS = '/sys/class/net/bonding_masters'
this is better kept private to a function, and could use a better name.

 MASTERS_FILE

maybe?
Line 32: 
Line 33: 
Line 34: def _exec_command(argv):
Line 35:     """


Line 44:                              stderr=subprocess.PIPE)
Line 45:         out, err = p.communicate()
Line 46:         rc = p.returncode
Line 47:     except:
Line 48:         logging.error(traceback.format_exc())
converting exceptions into return value is not very pythonic.

why don't you

 from vdsm.utils import execCmd

?
Line 49: 
Line 50:     return (rc, out, err)
Line 51: 
Line 52: 


Line 54:     devs = []
Line 55: 
Line 56:     if os.path.exists(DEVS):
Line 57:         with open(DEVS, 'r') as f:
Line 58:             devs = f.read().strip('\n').split(' ')
please consider an implementation like this:

 existing = set(file(MASTERS).read().split())

 with open(MASTERS, 'w') as f:
    for bond in REQUIRED - existing:
       f.write('+%s\n' % bond)
Line 59: 
Line 60:     # RHEV-M currently assumes that all bonding devices pre-exist
Line 61:     for bond in BOND:
Line 62:         if bond not in devs:


Line 70:     Load needed modules
Line 71:     """
Line 72: 
Line 73:     for mod in ['tun', 'bonding', '8021q']:
Line 74:         _exec_command([EX_MODPROBE, '%s' % mod])
why bother with

 '%s' % mod == mod

?


--
To view, visit http://gerrit.ovirt.org/7487
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icdc8f9d07a7150c4a846228811d6c5dcb17fa358
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi at linux.vnet.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Wenyi Gao <wenyi at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list