Francesco Romani has posted comments on this change. Change subject: vm.py: Always use 'qemu' domain type in fake mode ......................................................................
Patch Set 1: (2 comments) http://gerrit.ovirt.org/#/c/23938/1/tests/vmTests.py File tests/vmTests.py: Line 629: @MonkeyPatch(libvirtconnection, 'get', lambda x: ConnectionMock()) Line 630: @MonkeyPatch(utils, 'getHostUUID', Line 631: lambda: "fc25cbbe-5520-4f83-b82e-1541914753d9") Line 632: def testBuildCmdLinePPC64(self): Line 633: self.assertBuildCmdLine(CONF_TO_DOMXML_PPC64) Unrelated to patch, just noticed that the amount of @MonkeyPatch here is starting to be worrysome. However, stuff for next changes. http://gerrit.ovirt.org/#/c/23938/1/vdsm/vm.py File vdsm/vm.py: Line 915: Line 916: self.doc = xml.dom.minidom.Document() Line 917: Line 918: if not config.getboolean('vars', 'fake_kvm_support') and \ Line 919: utils.tobool(self.conf.get('kvmEnable', 'true')): I'm OK with the current code. Just a proposal, feel free to ignore. I think a little helper here could be nice, something along those lines: def has_kvm(self): if not utils.tobool(self.conf.get('kvmEnable', 'true')): return False if config.getboolean('vars', 'fake_kvm_support'): return False return True could also use a single if/and, but I', not a big fan of double negative-ish (not-fake_kvm_support) Line 920: domainType = 'kvm' Line 921: else: Line 922: domainType = 'qemu' Line 923: -- To view, visit http://gerrit.ovirt.org/23938 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I875854ccc4d884aa36a77cd062521889622e567d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
