Nir Soffer 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 created so many of them. 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? This looks like utility for extracting drive info from xml, so I think it is better use as a function in this module. Also, it would be nice to use lower case names when you move this method. Finally, class methods should be grouped at the start of the class, not spread randomly. 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 module. 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
