Change in vdsm[master]: [WIP] Move vdsmd.init to vdsm-tool

agl at us.ibm.com agl at us.ibm.com
Thu May 24 14:45:07 UTC 2012


Adam Litke has posted comments on this change.

Change subject: [WIP] Move vdsmd.init to vdsm-tool
......................................................................


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

(17 inline comments)

Need to convert more shell commands to python equivalents.

....................................................
File vdsm-tool/vdsmd.py
Line 49: EX_AWK = constants.EXT_AWK
In python, do not call out to awk, sed, grep, chmod, chown, tail, etc.  These functions all have easy python equivalents that are more efficient and which integrate better into the program.  I will identify the ones I see in this file and suggest some alternatives...

Line 138:     _exec_command([EX_SED, '-i', '--copy', '/%s/d' % BY_VDSM, file])
In this one you would want something like:

newfile = []
with open(file) as f:
    for line in f.readlines():
        if line.endswith(BY_VDSM):
            continue
        newfile.append(line)
with open(file, "w") as f:
    f.writelines(newfile)

Line 142:     rc = _exec_command([EX_GREP, '-q', '^\s*%s\s*=' % key, file])[2]
Here you would read the file with:

with open(file) as f:
    str = f.read()

Then search for your config entry:

if re.search('^\s*%s\s*=' % key, str) is None:
 ... do the f.write below

Line 149:     if _exec_command([EX_GREP, '-q', '%s' % BY_VDSM, file])[2] == 0:
Use the read and re example above here too.

Line 156:     out = _exec_command([GET_CONF_ITEM, file, section, item, default])[0]
The above is a simple python script.  Now that your program is python, you can just copy the code from that script into this function.

Line 187:     p1 = subprocess.Popen(['/bin/netstat', '-ntl'], stdout=subprocess.PIPE)
Calling netstat is fine, but you should capture its output into a string and use re to parse that string instead of calling grep.

Line 202:         _exec_command([EX_CHOWN, 'vdsm:kvm', path])
See python's os.chown()

http://docs.python.org/library/os.html

Line 232: 
All of these mk_*_path functions are pretty similar.  Please collapse them into a single function:

def _mk_vdsm_path(path, mode, user, group, restorecon=False):
    ...

See os.chmod, os.chown
http://docs.python.org/library/grp.html
http://docs.python.org/library/pwd.html

Is is ok to call restorecon

Line 241:                           stdin=p2.stdout, stdout=subprocess.PIPE)
read the file.
use re.search to get a match object
get the last match by using index -1 into the match object
You can use a grouped regular expression to easily extract the configuration value.

See http://docs.python.org/library/re.html#match-objects

Line 306:                           stdout=subprocess.PIPE, env=environ)
You know how to eliminate grep now.

Line 324:                           stdout=subprocess.PIPE)
Awk just splits a string by whitespace.  In python do this instead:


# read the output of df into str

# Get the last line

lastline = str.splitlines()[-1]


# Then get the 4th field

lastline.split()[3]


That's it!

Line 402:                   '"1:libvirt 3:event 3:json 1:util 1:qemu"')
We can make this more efficient that the original shell script by setting all of the conf items at the same time.  Do do this, you would pass a dictionary of config params to set into _set_vdsm_conf() and it should add all of them in one shot.

Line 412:             ssl == 'ture'):
ssl == 'true'

Line 425:         pass
You are silently ignoring lots of potential errors.  You probably mean to ignore only the error if the file does not exist.

Line 430:                          llogr])[0]
Read the file into an array like one of my previous examples.  Then only keep the lines that are not between '#vdsm and # end vdsm'.  Finally, write out the file again.

Line 477:         os.rename('/etc/sysctl.conf.vdsm', sysconf)
You should be able to replicate this file editing...

Line 513:     p2.stdout.close()
No grep or tail please.

--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Wenyi Gao <wenyi at linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Ryan Harper <ryanh at us.ibm.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming at linux.vnet.ibm.com>


More information about the vdsm-patches mailing list