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

danken at redhat.com danken at redhat.com
Tue Aug 28 13:44:32 UTC 2012


Dan Kenigsberg has posted comments on this change.

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


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

(5 inline comments)

....................................................
File vdsm-tool/load_needed_modules.py.in
Line 29: 
Line 30: 
Line 31: def _exec_command(argv):
Line 32:     subprocess.Popen(argv, stdout=subprocess.PIPE,
Line 33:                      stderr=subprocess.PIPE).communicate()
ignoring errors and returning None is not a good practice.
Line 34: 
Line 35: 
Line 36: def _is_bond_dev_available(bond):
Line 37:     if os.path.exists(DEVS):


Line 46: def load_needed_modules():
Line 47:     """
Line 48:     Load needed modules
Line 49:     """
Line 50:     _exec_command([EX_MODPROBE, 'tun'])
this begs for a 'for' loop
Line 51:     _exec_command([EX_MODPROBE, 'bonding'])
Line 52: 
Line 53:     for bond in BOND:
Line 54:         if not _is_bond_dev_available(bond):


Line 49:     """
Line 50:     _exec_command([EX_MODPROBE, 'tun'])
Line 51:     _exec_command([EX_MODPROBE, 'bonding'])
Line 52: 
Line 53:     for bond in BOND:
this is an ugly hack for backward compatibility. please keep the comment saying so. Actually, moving to a separate function is even better.
Line 54:         if not _is_bond_dev_available(bond):
Line 55:             with open(DEVS, 'w') as f:
Line 56:                 f.write('+%s\n' % bond)
Line 57: 


Line 50:     _exec_command([EX_MODPROBE, 'tun'])
Line 51:     _exec_command([EX_MODPROBE, 'bonding'])
Line 52: 
Line 53:     for bond in BOND:
Line 54:         if not _is_bond_dev_available(bond):
btw, this function is currently written in a very bashy way. let us use the benefits of Python: read all available bonds as a set, compare to the set of legacy bonds, and create what's needed.
Line 55:             with open(DEVS, 'w') as f:
Line 56:                 f.write('+%s\n' % bond)
Line 57: 


....................................................
File vdsm-tool/Makefile.am
Line 28: dist_vdsmtool_DATA = \
Line 29: 	__init__.py \
Line 30: 	passwd.py \
Line 31: 	validate_ovirt_certs.py \
Line 32: 	load_needed_modules.py
please end with

 $(NULL)

and keep sorted.


--
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: 3
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