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