Adam Litke has posted comments on this change.
Change subject: Move vdsmd.init to vdsm-tool
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(17 inline comments)
Thanks for taking my advice and reducing the number of external commands you are
executing. It is looking much better but there is still some work left to do.
....................................................
File vdsm/constants.py.in
Line 95
Did you mean to remove this? Seems fishy to me.
....................................................
File vdsm-tool/vdsmd.py
Line 71: VDSM_VER = '4.9.6'
This needs to come from dsaversion.py. Otherwise we have to remember to update this file
each time the vdsm version changes.
Line 113: return (out, err, rc)
Hmm, this looks wrong to me. If there is an exception in line 107 you will still return
rc = 0 and no output. If the program fails to run I think you want the exception to
propagate up to the caller of this.
Line 128: print >> sys.stderr, "Execution failed:", e
Why are you using print here and logging above? If there is a good reason, please add a
comment here to explain why.
Line 130: return rc
rc will be undefined if an exception was raised. This will cause a new exception. You
need to rework your exception handling logic. You can move the return statement inside
the try block. If you do this, then you need to figure out how you want to handle the
exception. Perhaps you log the error and return -1?
Line 157: skip = False
Line 157 is unreachable. Please remove it.
Line 182: lines.append(line)
This looks a lot like the function above. Please try to combine them into a single
function. You can pass the startPattern and endPattern as parameters.
Line 219: if os.path.exists('/etc/rhev-hypervisor-release'):
Does vdsm define a constant for this file name yet?
Line 221: elif not len(glob.glob('/etc/ovirt-node-*-release')) == 0:
How about this one?
Line 291: services = ['libvirt-guests']
I think this should be a constant at the top of the file.
Line 306: This fucntion test if the "/sbin/initctl" can be executed.
also s/test/tests/
Line 313: services = ['iscsid', 'multipathd', 'ntpd',
'wdmd', 'sanlock']
This should be another constant.
Line 343: p = subprocess.Popen([EX_DF, '-P', path], stdout=subprocess.PIPE)
It would be helpful if you add a docstring to show some example output from commands so
that we can more easily understand the parsing you are doing.
Line 358: def _is_bond_dev_available(bonddev):
Example command output please.
Line 475: ssl == 'true'):
Hmm, I like it. You learn something new every day! :)
Line 547: f.write('%s\n' % VDSM_SYSCTL_VER)
Please put these variables into a dict so that we can easily add/remove/update the
settings.
....................................................
File vdsm/vdsmd.init.in
Line 1
Nice!!
--
To view, visit
http://gerrit.ovirt.org/4738
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52db67207532ac7ded51cdd8c5dc388e85629104
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Wenyi Gao <wenyi(a)linux.vnet.ibm.com>