Nir Soffer has posted comments on this change. Change subject: virt: Make BIOS messages available on vmconsole ......................................................................
Patch Set 6: Code-Review-1 (5 comments) https://gerrit.ovirt.org/#/c/48404/6//COMMIT_MSG Commit Message: Line 8: Line 9: ovirt-vmconsole allows access to VMs via serial console. However, this Line 10: currently works only for processes running on the VM operating system, Line 11: such as getty. It would be useful if vmconsole interacted immediately Line 12: since the start of a VM. This is not clear - how do you wan to interact with the vm if not with some process running on it? What do you mean by interacted immediately? Line 13: 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. https://gerrit.ovirt.org/#/c/48404/6/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1592: for consoleDev in self._devices[hwclass.CONSOLE]: Line 1593: if consoleDev.isSerial: Line 1594: break Line 1595: else: Line 1596: consoleDev = None Please avoid for-else syntax, it is always confusing and unneeded. Please add instead a private method returning the the serial console device. Also, since consoleDev is always a serial console, why not call it serial_console? Line 1597: bios_use_serial = consoleDev is not None Line 1598: Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch) Line 1600: domxml.appendOs(bios_use_serial=bios_use_serial) Line 1593: if consoleDev.isSerial: Line 1594: break Line 1595: else: Line 1596: consoleDev = None Line 1597: bios_use_serial = consoleDev is not None This variable it not needed. Line 1598: Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch) Line 1600: domxml.appendOs(bios_use_serial=bios_use_serial) Line 1601: Line 1596: consoleDev = None Line 1597: bios_use_serial = consoleDev is not None Line 1598: Line 1599: domxml = vmxml.Domain(self.conf, self.log, self.arch) Line 1600: domxml.appendOs(bios_use_serial=bios_use_serial) This should be: bios_use_serial=consoleDev is not None Line 1601: Line 1602: if self.arch == caps.Architecture.X86_64: Line 1603: osd = caps.osversion() Line 1604: https://gerrit.ovirt.org/#/c/48404/6/vdsm/virt/vmxml.py File vdsm/virt/vmxml.py: Line 230: self.dom.setAttr('xmlns:' + METADATA_VM_TUNE_PREFIX, Line 231: METADATA_VM_TUNE_URI) Line 232: self.dom.appendChild(self._metadata) Line 233: Line 234: def appendOs(self, bios_use_serial=False): Do we have other useserial attributes in this element? Does the caller care about the bios element, or about using a serial console? If the later is more correct, use_serial_console is a better argument name. Line 235: """ Line 236: Add <os> element to domain: Line 237: Line 238: <os> -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches