Francesco Romani has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole ......................................................................
Patch Set 1: (7 comments) https://gerrit.ovirt.org/#/c/48404/1//COMMIT_MSG Commit Message: Line 14: This change is the first step towards the goal. It enables BIOS serial Line 15: console interaction in a VM's XML domain description when a serial Line 16: console is available. Line 17: Line 18: Note that at least oVirt Engine 3.6 snapshot is required to make the this is true for oVirt/RHEV. For VDSM, is sufficient that the console is 'type'='serial'. I think you should either mention both or just the latter in the commit message. Line 19: functionality actually working. The reason is that access to BIOS Line 20: messages requires presence of serial (not virtio) console. Line 21: Line 22: Change-Id: Ifc560d2562a913b7dba9a5c01952429459595973 https://gerrit.ovirt.org/#/c/48404/1/tests/vmTests.py File tests/vmTests.py: 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 testOSXMLBootMenu, which is more focused and doesn't test internal methods (_buildDomainXML) 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 1: # Line 2: # Copyright 2008-2015 Red Hat, Inc. correct, but unneeded here Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or Line 1590: Line 1591: def _buildDomainXML(self): Line 1592: for consoleDev in self._devices[hwclass.CONSOLE]: Line 1593: if consoleDev.isSerial: Line 1594: break nice refactoring, but not needed for this patch. Line 1595: else: Line 1596: consoleDev = None Line 1597: self.conf['_serialConsoleAvailable'] = consoleDev is not None Line 1598: 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. _hidden attributes are particularly troublesome, especially if short lived. We need a better way to notify appendOs here. 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 1: # Line 2: # Copyright 2008-2015 Red Hat, Inc. correct, but unneeded for this patch. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or 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? 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: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
