Change in vdsm[master]: replace configure_libvirt.py with python code.

mtayer at redhat.com mtayer at redhat.com
Sun Jun 8 16:14:26 UTC 2014


mooli tayer has posted comments on this change.

Change subject: replace configure_libvirt.py with python code.
......................................................................


Patch Set 23:

(8 comments)

http://gerrit.ovirt.org/#/c/27298/23//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: replace configure_libvirt.py with python code.
Line 8: 
Line 9: The port is to behave as similar as possible to the old
Line 10: sh code. python should allow easier testing (tests added
> Seems like ) got moved around
fixed
Line 11: ) and better overall readability.
Line 12: 
Line 13: A testing matrix [1] documents my tests of this feature.
Line 14: 


http://gerrit.ovirt.org/#/c/27298/23/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 30: from .. import utils
Line 31: from . import service, expose, validate_ovirt_certs
Line 32: from . import NotRootError, UsageError
Line 33: from configfile import ConfigFile, ParserWrapper
Line 34: from ..constants import QEMU_PROCESS_GROUP, \
> If you are already working on this line please change it to
from .. constants import \
   QEMU_PROCESS_GROUP, \
   SANLOCK_USER, \
   ...
?
Line 35:     SANLOCK_USER, VDSM_GROUP, SYSCONF_PATH, P_VDSM_CERT, \
Line 36:     SANLOCK_ENABLED, LIBVIRT_SELINUX, P_VDSM
Line 37: from vdsm.config import config
Line 38: 


Line 35:     SANLOCK_USER, VDSM_GROUP, SYSCONF_PATH, P_VDSM_CERT, \
Line 36:     SANLOCK_ENABLED, LIBVIRT_SELINUX, P_VDSM
Line 37: from vdsm.config import config
Line 38: 
Line 39: if utils.isOvirtNode:
> Again, I think it's a function.
changed.
Line 40:     from ovirt.node.utils.fs import Config
Line 41: 
Line 42: 
Line 43: class InvalidConfig(UsageError):


Line 117:         # write configuration
Line 118:         for cfile, content in self.FILES.items():
Line 119:             content['configure'](self, content, vdsmConfiguration)
Line 120: 
Line 121:         sys.stdout.write("Reconfiguration of libvirt is done.")
> end with \n?
Added \n, also notice this is moved to verb level here:
 http://gerrit.ovirt.org/#/c/27841/
Line 122: 
Line 123:     def validate(self):
Line 124:         """
Line 125:         Validate conflict in configured files


Line 179: LIBVIRTD_UPSTART
> os.path.basename(fname) == LIBVIRTD_UPSTART
Done.


Line 183:             if os.path.isfile(packaged):
Line 184:                 if not os.path.isfile(TARGET):
Line 185:                     service.service_stop('libvirtd')
Line 186:                 if (not os.path.isfile(TARGET) or
Line 187:                         not filecmp.cmp(packaged, TARGET)):
> if not os.path.isfile(TARGET):
iiuc what you are saying would be true if it was:
not os.path.isfile(TARGET) and not filecmp.cmp(packaged, TARGET)

I think 
if a():
  x()
if a or b:
  y()

is not the same as

if a:
  x()
  if b:
    y()

In the case where a = False and b = True.
Line 188:                     oldmod = None
Line 189:                     if os.path.isfile(TARGET):
Line 190:                         oldmod = os.stat(TARGET).st_mode
Line 191: 


Line 197: 
Line 198:                     if (oldmod is not None and
Line 199:                             oldmod != os.stat(TARGET).st_mode):
Line 200:                         os.chmod(TARGET, oldmod)
Line 201:                     rc, out, err = utils.execCmd((INITCTL,
> Factor out to a proper function
Done
Line 202:                                                   "reload-configuration"))
Line 203:                     if rc != 0:
Line 204:                         sys.stdout.write(out)
Line 205:                         sys.stderr.write(err)


Line 215:         ssl = config.getboolean('vars', 'ssl')
Line 216: 
Line 217:         lconf_p = ParserWrapper({
Line 218:             'listen_tcp': '0',
Line 219:             'auth_tcp': 'sasl'
> Add comma at EOL
Done
Line 220:         })
Line 221:         lconf_p.read(self._getFile('LCONF'))
Line 222:         listen_tcp = lconf_p.getint('listen_tcp')
Line 223:         auth_tcp = lconf_p.get('auth_tcp')


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74bfe05bb4b5f5d09021f21b324f9b7d5d0fdaab
Gerrit-PatchSet: 23
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: automation at ovirt.org
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