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@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Wenyi Gao wenyi@linux.vnet.ibm.com