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