Milan Zamazal has posted comments on this change. Change subject: virt: Move Vm._getUnderlyingDriveInfo() out of Vm ......................................................................
Patch Set 11: (3 comments) https://gerrit.ovirt.org/#/c/53677/11/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1723: devices = self._devices Line 1724: vmdevices.network.Interface.update_device_info( Line 1725: self, devices[hwclass.NIC]) Line 1726: vmdevices.storage.Drive.update_device_info( Line 1727: self, devices[hwclass.DISK]) > I don't like these class methods, but it seems to be too late now after you Why don't you like them? Because they are class methods or for another reason? We can make changes in followup patches. Line 1728: vmdevices.core.Sound.update_device_info( Line 1729: self, devices[hwclass.SOUND]) Line 1730: vmdevices.core.Video.update_device_info( Line 1731: self, devices[hwclass.VIDEO]) https://gerrit.ovirt.org/#/c/53677/11/vdsm/virt/vmdevices/storage.py File vdsm/virt/vmdevices/storage.py: Line 443: dom = ET.fromstring(xml_string) Line 444: return bool(dom.findall(self._xpath)) Line 445: Line 446: @classmethod Line 447: def _getDriveIdentification(cls, dom): > I don't see any use of the cls argument - why is this a classmethod? Done Line 448: sources = dom.getElementsByTagName('source') Line 449: if sources: Line 450: devPath = (sources[0].getAttribute('file') or Line 451: sources[0].getAttribute('dev') or Line 457: alias = dom.getElementsByTagName('alias')[0].getAttribute('name') Line 458: return alias, devPath, name Line 459: Line 460: @classmethod Line 461: def update_device_info(cls, vm, device_conf): > Same, I don't see any use of this class, so it should be a function in this Each device type has its own update_device_info method, so it should be in the corresponding class. We can argue whether it should be classmethod or staticmethod, but conceptually it's not a separate function. Line 462: # FIXME! We need to gather as much info as possible from the libvirt. Line 463: # In the future we can return this real data to management instead of Line 464: # vm's conf Line 465: for x in vm.domain.get_device_elements('disk'): -- To view, visit https://gerrit.ovirt.org/53677 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib6b372f86b82da7422727f3d084b9afc5505a289 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@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
