Change in vdsm[master]: Adding remove\disable verbs to vdsm-tool for admin usages

mtayer at redhat.com mtayer at redhat.com
Tue Feb 25 16:30:07 UTC 2014


mooli tayer has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 1:

(8 comments)

I am continuing Yaniv's work here.

http://gerrit.ovirt.org/#/c/21772/1/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 48: 
Line 49:     def isconfigured(self):
Line 50:         return True
Line 51: 
Line 52:     def removeConf(self, filename, beginField=None, endField=None):
> this is not removeConf in inheritance model, it is utility, the removeConf 
Done
Line 53:         newfile = []
Line 54:         if not beginField and not endField:
Line 55:             return
Line 56:         else:


Line 52:     def removeConf(self, filename, beginField=None, endField=None):
Line 53:         newfile = []
Line 54:         if not beginField and not endField:
Line 55:             return
Line 56:         else:
> so why else?
Done
Line 57:             with open(filename, 'r') as f:
Line 58:                 skip = False
Line 59:                 for line in f.readlines():
Line 60:                     if beginField and endField:


Line 70:                             continue
Line 71:                     else:
Line 72:                         if line.endswith('%s\n' % endField):
Line 73:                             continue
Line 74:                     newfile.append(line)
> we are talked about this structure... it is spaghetti!
Done
Line 75:             tmp_file = tempfile.NamedTemporaryFile(delete=False)
Line 76:             tname = tmp_file.name
Line 77:             tmp_file.writelines(newfile)
Line 78:             tmp_file.close()


Line 103:         for path in conf_paths:
Line 104:             try:
Line 105:                 super(LibvirtModuleConfigure, self).removeConf(path,
Line 106:                                                                conf_prefix,
Line 107:                                                                conf_suffix)
> please avoid drawing code...
Replaced with utility method
Line 108:             except OSError, e:
Line 109:                 if e.errno != os.errno.EEXIST:
Line 110:                     raise
Line 111: 


Line 106:                                                                conf_prefix,
Line 107:                                                                conf_suffix)
Line 108:             except OSError, e:
Line 109:                 if e.errno != os.errno.EEXIST:
Line 110:                     raise
> you should first check if file exist then attempt to modify, do not assume 
Not sure what you mean here.
Line 111: 
Line 112:         utils.rmFile(P_SYSTEMCTL_CONF)
Line 113: 
Line 114:     def _exec_libvirt_configure(self, action):


Line 320: """
adding
ret = True


http://gerrit.ovirt.org/#/c/21772/1/lib/vdsm/tool/passwd.py
File lib/vdsm/tool/passwd.py:

Line 52:     Remove vdsm password for libvirt connection
Line 53:     """
Line 54:     script = ['/usr/sbin/saslpasswd2', '-p', '-a', 'libvirt', '-d',
Line 55:               constants.SASL_USERNAME]
Line 56:     p = subprocess.Popen(script)
> why do you need this script temp variable? why isn't it tuple?
done
Line 57:     output, err = p.communicate()
Line 58:     if p.returncode != 0:


http://gerrit.ovirt.org/#/c/21772/1/lib/vdsm/tool/seboolsetup.py
File lib/vdsm/tool/seboolsetup.py:

Line 65:     """Disable the required selinux booleans"""
Line 66:     setup_booleans(False)
Line 67: 
Line 68: 
Line 69: @expose("clear-selinux-policy")
> sebool_unconfig does the same. I'll remove that
Removing section
Line 70: def clear_selinux_policy():
Line 71:     """
Line 72:     Clear the changes of selinux policy
Line 73:     """


-- 
To view, visit http://gerrit.ovirt.org/21772
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list