Change in vdsm[master]: vmdevices: test moving getUnderlying*Info

fromani at redhat.com fromani at redhat.com
Fri Mar 13 13:39:04 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: vmdevices: test moving getUnderlying*Info
......................................................................


Patch Set 2:

(4 comments)

https://gerrit.ovirt.org/#/c/38402/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4057
Line 4058
Line 4059
Line 4060
Line 4061
> Removing this ofc will force you to have it also either replace it everywhe
or make this method a wrapper to new function for the time being. Which is ugly, but bearable, and will allow us to make smaller commits.


https://gerrit.ovirt.org/#/c/38402/2/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:

Line 134:         sound.setAttrs(model=self.device)
Line 135:         return sound
Line 136: 
Line 137:     @staticmethod
Line 138:     def from_xml(domain_xml, device_configs, device_objects):
this is very nice, and I think it is the right direction.

But designwise, code's asymmetrical.
When we construct devices, we iterate through conf, and each device builds it's own XML, which is then joined by the orchestrating functions.

Here's the other way around, each device staticmethod does the dispatch to configuration and objects.

We can and should go a step further. Devices should be able to be fed with their XML representation and update themselves. Code in Vm class (or, better, elsewhere) should take care of dispatching xml snippets to devices, and then moving back updated devices into Vm objects.
Line 139:         """
Line 140:         Obtain sound devices info from libvirt xml.
Line 141:         """
Line 142:         for device_xml in domain_xml.get_device_elements('sound'):


Line 138:     def from_xml(domain_xml, device_configs, device_objects):
Line 139:         """
Line 140:         Obtain sound devices info from libvirt xml.
Line 141:         """
Line 142:         for device_xml in domain_xml.get_device_elements('sound'):
so, this loop should be in the caller. Well, since we want to insulate Vm class from XML, maybe we need an intermediate caller to orchestrate the dispatching. But I can get away with this, and this could be a second step.
Line 143:             alias = device_xml.getElementsByTagName('alias')[0].\
Line 144:                 getAttribute('name')
Line 145:             # Get sound card address
Line 146:             address = vmxml.get_device_address_from_xml(device_xml)


Line 144:                 getAttribute('name')
Line 145:             # Get sound card address
Line 146:             address = vmxml.get_device_address_from_xml(device_xml)
Line 147: 
Line 148:             for sound_device in device_objects[hwclass.SOUND]:
not for this patch/first step: this is a half a lie. We are just looking for the first (and only) sound device, and this intent should be clearer.
Line 149:                 if not hasattr(sound_device, 'address') or not \
Line 150:                         hasattr(sound_device, 'alias'):
Line 151: 
Line 152:                     sound_device.alias = alias


-- 
To view, visit https://gerrit.ovirt.org/38402
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41caf127a8d3c674db98d5334f927233fa2c7c99
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list