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

dougsland at redhat.com dougsland at redhat.com
Tue May 20 04:59:54 UTC 2014


Douglas Schilling Landgraf has posted comments on this change.

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


Patch Set 14:

(11 comments)

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

Line 5: CommitDate: 2014-05-19 13:56:56 +0300
Line 6: 
Line 7: replace configure_libvirt.py with python code.
Line 8: 
Line 9: The port is to behave as simillar as possible and will be tested
minor typo: similar
Line 10: to configure/remove configuration created by configure_libvirt.sh.
Line 11: 
Line 12: The new code uses temp files and replace instead of line by line
Line 13: editing for durability.


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

Line 20: import errno
Line 21: import os
Line 22: import sys
Line 23: import grp
Line 24: import argparse
imports alphabetically?
Line 25: import filecmp
Line 26: import rpm
Line 27: import shutil
Line 28: import traceback


Line 125:         """
Line 126:         for path in (self._getPersistedFiles()):
Line 127:             if not self._openConfig(path).hasConf():
Line 128:                 return False
Line 129:         return True
In _isSslConflict() I see you only using one return statement, what about use it here as well?
Line 130: 
Line 131:     def removeConf(self):
Line 132:         for cfile, content in LibvirtModuleConfigure.FILES.items():
Line 133:             content['removeConf'](self, content['path'])


Line 160:             # this could happen in Ubuntu or other distro,
Line 161:             # so continue to use system default init mechanism
Line 162:             for filename in iterateLibvirtFiles():
Line 163:                 if LIBVIRTD_UPSTART in filename:
Line 164:                     packeged = filename
Is it a typo for packaged ?
Line 165:                     break
Line 166: 
Line 167:             if packeged is not None and os.path.isfile(packeged):
Line 168:                 if not os.path.isfile(TARGET):


Line 163:                 if LIBVIRTD_UPSTART in filename:
Line 164:                     packeged = filename
Line 165:                     break
Line 166: 
Line 167:             if packeged is not None and os.path.isfile(packeged):
if packeged is not assigned for some reason in the above for/if I don't think it will be None and even exist in the stack.
Line 168:                 if not os.path.isfile(TARGET):
Line 169:                     service.service_stop('libvirtd')
Line 170:                 if (not os.path.isfile(TARGET) or
Line 171:                         not filecmp.cmp(packeged, TARGET)):


Line 183:                         sys.stderr.write(err)
Line 184:                         raise RuntimeError(
Line 185:                             "Failed to reload upstart configuration.")
Line 186: 
Line 187:     def _isSslConflict(self):
I see you used docstring to others methods, is it possible to add docstring here as well? Please consider to removeConf() and _getPersistedFiles() too.
Line 188:         config.read(self.getFile('VDSM_CONF'))
Line 189:         ssl = config.getboolean('vars', 'ssl')
Line 190: 
Line 191:         lconf_p = ParserWrapper({


Line 243: 
Line 244:     def _openConfig(self, path):
Line 245:         return ConfigFile(path, self.CONF_VERSION)
Line 246: 
Line 247:     # On configure functions
Should this comment be part of docstring?
Line 248:     def _addSection(self, content, vdsmConfiguration):
Line 249:         """
Line 250:         Add a 'configuration section by vdsm' part to a config file.
Line 251:         This section contains only keys not originally defined


Line 280:         except OSError as e:
Line 281:             if e.errno != errno.ENOENT:
Line 282:                 raise
Line 283: 
Line 284:     # On removeConf functions
Should this comment be part of the below docstring?
Line 285:     def _unprefixAndRemoveSection(self, path):
Line 286:         """
Line 287:         undo changes done by _prefixAndPrepend.
Line 288:         """


Line 304:     # on every update. see 'configuration versioning:' at ConfigFile.py for
Line 305:     # details.
Line 306:     CONF_VERSION = '4.13.0'
Line 307: 
Line 308:     PKI = os.path.join(SYSCONF_PATH, 'pki/vdsm')
following the naming schema, would be PKI_DIR?
Line 309:     CA_FILE = os.path.join(PKI, 'certs/cacert.pem')
Line 310:     CERT_FILE = os.path.join(PKI, 'certs/vdsmcert.pem')
Line 311:     KEY_FILE = os.path.join(PKI, 'keys/vdsmkey.pem')
Line 312:     LS_CERT_DIR = os.path.join(PKI, 'libvirt-spice')


Line 339:                 {
Line 340:                     'conditions': {},
Line 341:                     'content': {
Line 342:                         'listen_addr': '"0.0.0.0"',
Line 343:                         'unix_sock_group': '"qemu"',
Is it possible to use QEMU_PROCESS_GROUP constant here?
Line 344:                         'unix_sock_rw_perms': '"0770"',
Line 345:                         'auth_unix_rw': '"sasl"',
Line 346:                         'host_uuid': uuid.uuid4(),
Line 347:                         'keepalive_interval': -1,


Line 754:                 c.removeConf()
Line 755:             except Exception:
Line 756:                 sys.stderr.write('cannot remove %s\n' % c.getName())
Line 757:                 traceback.print_exc(file=sys.stderr)
Line 758:                 failed = True
shouldn't we validate if it's ovirtNode and persist at this point?
Line 759:     if failed:
Line 760:         raise RuntimeError("Remove configuration failed")
Line 761: 
Line 762: 


-- 
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: 14
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: 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