Milan Zamazal has posted comments on this change. Change subject: virt: Move Vm._getUnderlyingDriveInfo() out of Vm ......................................................................
Patch Set 11: (1 comment) 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 them because updating the conf/devices from xml should not be > I don't like them because updating the conf/devices from xml should not be > the responsibility of the Device class. I see. I think it's desirable to group code by devices. Most device classes just create and process domain XML and it wouldn't be good to perform those two transformations in separate places. Properties of a given device should be handled in a single place, as a form of encapsulation. > Also in this design the code for handling the xml is spread all over the > place, instead of one module handling this task. We should generally improve XML handling, it's mess. If we can do that then this shouldn't be a problem. 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]) -- 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
