Change in vdsm[master]: replace configure_libvirt.py with python code.
mtayer at redhat.com
mtayer at redhat.com
Tue May 13 11:43:00 UTC 2014
mooli tayer has posted comments on this change.
Change subject: replace configure_libvirt.py with python code.
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.ovirt.org/#/c/27298/6/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 98: 'path': os.path.join(
Line 99: SYSCONF_PATH,
Line 100: 'libvirt/libvirtd.conf'
Line 101: ),
Line 102: 'configureFunc': 'addSection',
> why do you care if function is above :)
Done
Line 103: 'removeConfFunc': 'removeSection',
Line 104: 'fragments': [
Line 105: {
Line 106: 'conditions': {},
Line 311: }
Line 312: }
Line 313:
Line 314: def __init__(self):
Line 315: super(LibvirtModuleConfigure, self).__init__()
> This __init__ is useless - if you remove it, the __init__() method you inhe
checked and done.
Line 316:
Line 317: def getName(self):
Line 318: return 'libvirt'
Line 319:
Line 317: def getName(self):
Line 318: return 'libvirt'
Line 319:
Line 320: def getFile(self, fname):
Line 321: return LibvirtModuleConfigure.FILES[fname]['path']
> Use self.FILES - class attributes are available to an instance in Python.
Done
Line 322:
Line 323: def getServices(self):
Line 324: return ["supervdsmd", "vdsmd", "libvirtd"]
Line 325:
Line 373: context = {
Line 374: 'certs_exist': all(os.path.isfile(f) for f in [
Line 375: LibvirtModuleConfigure.CA_FILE,
Line 376: LibvirtModuleConfigure.CERT_FILE,
Line 377: LibvirtModuleConfigure.KEY_FILE
> self would be nicer.
Done
Line 378: ]),
Line 379: 'ssl_enabled': config.getboolean('vars', 'ssl'),
Line 380: 'sanlock_enabled': SANLOCK_ENABLED,
Line 381: 'libvirt_selinux': LIBVIRT_SELINUX
Line 403: lambda cfile: (
Line 404: cfile['configureFunc'] == 'prefixAndPrepend' or
Line 405: cfile['configureFunc'] == 'addSection'),
Line 406: LibvirtModuleConfigure.FILES.values()
Line 407: )
> It is not clear what you are trying to do here; While Python support this f
Done +
alon's comment about using functions and not function names.
Line 408: )
Line 409:
Line 410: def _sysvToUpstart(self):
Line 411: """
Line 416: INITCTL = '/sbin/initctl'
Line 417: LIBVIRTD_UPSTART = 'libvirtd.upstart'
Line 418: TARGET = os.path.join(SYSCONF_PATH, "init/libvirtd.conf")
Line 419:
Line 420: if os.path.isfile(INITCTL) and os.access(INITCTL, os.X_OK):
> os.access uses the real uid/gid, so if someone run this code using sudo, th
This code always runs as root so I think this is enough here.
Line 421: ts = rpm.TransactionSet()
Line 422: mi = itertools.chain(*[ts.dbMatch('name', name)
Line 423: for name in ['libvirt', 'libvirt-daemon']])
Line 424: # libvirtd package does not provide libvirtd.upstart,
Line 425: # this could happen in Ubuntu or other distro,
Line 426: # so continue to use system default init mechanism
Line 427:
Line 428: for filename in itertools.chain(*[h[rpm.RPMTAG_FILENAMES]
Line 429: for h in mi]):
> Two chains with these cryptic variable names make it too hard to understand
Yes it is nicer.
Line 430: if LIBVIRTD_UPSTART in filename:
Line 431: packeged = filename
Line 432: break
Line 433:
--
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: 6
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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.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