Alon Bar-Lev has posted comments on this change. Change subject: replace configure_libvirt.py with python code. ......................................................................
Patch Set 5: (10 comments) http://gerrit.ovirt.org/#/c/27298/5/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 37: SANLOCK_ENABLED, LIBVIRT_SELINUX Line 38: from vdsm.config import config Line 39: Line 40: Line 41: VDSM_CONF_SECTION = { when do you decide what to put at class level and what as global? I prefer class level if possible :) Line 42: 'sectionStart': '## beginning of configuration section by vdsm', Line 43: 'sectionEnd': '## end of configuration section by vdsm', Line 44: 'version': '4.13.0' Line 45: } Line 83: Line 84: # Libvirt daemon configuration Line 85: { Line 86: 'condition': lambda context: Line 87: True, > use variable. True doesn't mean anything I guess this what he meant, to execute it always. Line 88: 'conf': "LCONF", Line 89: 'content': { Line 90: 'listen_addr': '"0.0.0.0"', Line 91: 'unix_sock_group': '"qemu"', Line 210: Line 211: # sanlock configuration file Line 212: { Line 213: 'condition': lambda x: Line 214: SANLOCK_ENABLED, I suggest that for consistency you move all into context or as I see now the condition list which is much clearer, you can use dictionary key instead of lambda... as there is no logic. conditions = { 'sanlock_enabled': True, ... } { condition_key = 'xxxx', Line 215: Line 216: 'conf': "QLCONF", Line 217: 'content': { Line 218: 'auto_disk_leases': 0, Line 292: for item in self.CONF: Line 293: condition, conf, content = ( Line 294: item['condition'], item['conf'], item['content']) Line 295: if condition(c): Line 296: config_map[conf].update(content) any reason to use extensive temp vars? why not as simple as: for item in self.CONF: if item['condition'](c): config_map[item['conf']],update(item['content']) Line 297: Line 298: for name, configuration in config_map.items(): Line 299: with ConfigFile(self.envGet(name), **VDSM_CONF_SECTION) as conff: Line 300: for key, val in configuration.items(): Line 301: conff.addEntry(key, val) Line 302: Line 303: qnetwork = self.envGet('QNETWORK') Line 304: if os.path.exists(qnetwork): Line 305: os.remove(qnetwork) why is this an exception and cannot be done for all? Cant we have some key within the dictionary instructing to remove so we have no exceptions in logic? Line 306: Line 307: # libvirt log rotate configuration Line 308: # Todo: make idempotent Line 309: with ConfigFile( Line 308: # Todo: make idempotent Line 309: with ConfigFile( Line 310: self.envGet('LRCONF'), **VDSM_CONF_SECTION) as conf: Line 311: conf.prefixLines('# VDSM backup') Line 312: conf.prependSection(self.LLOGR_CONF) why is this exception? can we have dictionary entry of 'type' to prepend or addEntry and remove the explicit logic? Line 313: Line 314: if utils.isOvirtNode(): Line 315: from ovirtnode import ovirtfunctions Line 316: Line 320: self.envGet('LDCONF'), Line 321: self.envGet('QLCONF'), Line 322: self.envGet('LRCONF') Line 323: ): Line 324: ovirtfunctions.ovirt_store_config(fname) please have a list/set of all files that were updated and iterate through that list and not explicit hardcode this Line 325: Line 326: sys.stdout.write("Reconfiguration of libvirt is done.") Line 327: Line 328: def _sysvToUpstart(self): Line 353: if not os.path.isfile(TARGET): Line 354: service.service_stop('libvirtd') Line 355: if not os.path.isfile(TARGET) or \ Line 356: not filecmp.cmp(packeged, TARGET): Line 357: shutil.copyfile(packeged, TARGET) when you copy a file you need chmod as well. Line 358: rc, out, err = utils.execCmd((INITCTL, Line 359: "reload-configuration")) Line 360: if rc != 0: Line 361: sys.stdout.write(out) Line 391: "libvirtd.conf: listen_tcp=0, auth_tcp=\"sasl\", \n" Line 392: "qemu.conf: spice_tls=1.\n" Line 393: ) Line 394: return False Line 395: else: if you return at middle of function (look above) why do you need the else? please consider avoiding returning at middle, you can set ret = instead return and all will be super. Line 396: if listen_tcp == 1 and auth_tcp == 'none' and spice_tls == 0: Line 397: sys.stdout.write( Line 398: "SUCCESS: ssl configured to false. No conflicts.\n") Line 399: return True Line 433: self.envGet('LCONF'), Line 434: self.envGet('QCONF'), Line 435: self.envGet('LDCONF'), Line 436: self.envGet('QLCONF') Line 437: ): if you already have this envGet, why don't you mark what is for configure and return this list when instructed instead of hardcoding it over and over? Line 438: if os.path.exists(path): Line 439: with ConfigFile(path, **VDSM_CONF_SECTION) as conff: Line 440: conff.removeConf() Line 441: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
