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