Milan Zamazal has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole ......................................................................
Patch Set 1: (4 comments) https://gerrit.ovirt.org/#/c/48404/1/tests/vmTests.py File tests/vmTests.py: Line 1: # Line 2: # Copyright IBM Corp. 2012 Line 3: # Copyright 2013-2015 Red Hat, Inc. > correct, but unneeded for this patch. Are the copyright years in VDSM updated in a different way than on the first change in given file each year? Line 4: # Line 5: # This program is free software; you can redistribute it and/or modify Line 6: # it under the terms of the GNU General Public License as published by Line 7: # the Free Software Foundation; either version 2 of the License, or Line 1418: 'specParams': {'consoleType': console_type}}, Line 1419: with fake.VM(devices=devices, create_device_objects=True) as v: Line 1420: dom_xml = v._buildDomainXML() Line 1421: self.assertEqual('<bios useserial="yes"/>' in dom_xml, Line 1422: use_serial) > Kudos for the test. However, I'd prefer something more like The problem is that presence of useserial is dependent on presence of the serial console. So we have to invoke more of the XML domain building process to test it, not just the <os> part. Line 1423: Line 1424: Line 1425: def _load_xml(name): Line 1426: test_path = os.path.realpath(__file__) https://gerrit.ovirt.org/#/c/48404/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1593: if consoleDev.isSerial: Line 1594: break Line 1595: else: Line 1596: consoleDev = None Line 1597: self.conf['_serialConsoleAvailable'] = consoleDev is not None > we should avoid to add attributes to Vm.conf unless it is really needed. _h I agree, so let's try passing an argument to appendOs instead. Line 1598: Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch) Line 1600: domxml.appendOs() Line 1601: https://gerrit.ovirt.org/#/c/48404/1/vdsm/virt/vmxml.py File vdsm/virt/vmxml.py: Line 276: if utils.tobool(self.conf.get('bootMenuEnable', False)): Line 277: oselem.appendChildWithArgs('bootmenu', enable='yes') Line 278: Line 279: if self.conf.get('_serialConsoleAvailable'): Line 280: oselem.appendChildWithArgs('bios', useserial='yes') > what happens if we inconditionally enable useserial? If serial console is not available then libvirt complains and the VM doesn't start. Line 281: Line 282: def appendSysinfo(self, osname, osversion, serialNumber): Line 283: """ Line 284: Add <sysinfo> element to domain: -- To view, visit https://gerrit.ovirt.org/48404 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
