Change in vdsm[master]: Added support for before and after device create.

danken at redhat.com danken at redhat.com
Thu Mar 28 20:14:46 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: Added support for before and after device create.
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(4 inline comments)

....................................................
Commit Message
Line 5: CommitDate: 2013-03-28 16:48:09 +0200
Line 6: 
Line 7: Added support for before and after device create.
Line 8: 
Line 9: before_device_create is called for each device, right before
for each device, or for each device with custom properties(?)

I am slightly worried that running all the device-specific hook script for all devices is too wasteful (just think of the added log lines!) , as there are relatively many device per VM. 

I suggested to avoid that, and run the scripts only for devices that has custom properties. The down side of this is that it's slightly different from the per-vm sematics (hooks are run always, not only if custom properties are there). However, if you do not care about per-device custom properties, you can put your script in before_vm_create.
Line 10: before_vm_create. Each such hook is supplied the device's
Line 11: dom xml and any custom properties. after_device_create is
Line 12: called right after after_vm_start.
Line 13: 


....................................................
File vdsm/libvirtvm.py
Line 1318:         domxml.appendInput()
Line 1319:         domxml.appendGraphics()
Line 1320: 
Line 1321:         '''
Line 1322:             For each device
I love comments, but:
* translating Python into English like you do here, ends up in trouble, as the comment/code quickly fall out of sync.
* use docstring to document functions. Comments are written with #
Line 1323:                 Get its XML, run a before_device_create hook
Line 1324:                 Append the XML to the VM's DOM XML
Line 1325:                 Save the XML for future use
Line 1326:         '''


Line 1325:                 Save the XML for future use
Line 1326:         '''
Line 1327:         for devType in self._devices:
Line 1328:             for dev in self._devices[devType]:
Line 1329:                 devxmlString = dev.getXML().toxml(encoding='utf-8')
it's not very pythonic to include the type in the variable name. if devElem is holding the object form, maybe devxml is now free for the string.

also, it's confusing to have self._deviceXML cache devxmlString.
self._devxml and devxml seems better to me.
Line 1330:                 devxmlString = hooks.before_device_create(
Line 1331:                     devxmlString, self.conf, getattr(dev, 'custom', {}))
Line 1332:                 dev._deviceXML = devxmlString
Line 1333:                 devxml = xml.dom.minidom.parseString(devxmlString).firstChild


Line 1329:                 devxmlString = dev.getXML().toxml(encoding='utf-8')
Line 1330:                 devxmlString = hooks.before_device_create(
Line 1331:                     devxmlString, self.conf, getattr(dev, 'custom', {}))
Line 1332:                 dev._deviceXML = devxmlString
Line 1333:                 devxml = xml.dom.minidom.parseString(devxmlString).firstChild
why did you rename devElem to devxml ?
Line 1334:                 domxml._devices.appendChild(devxml)
Line 1335: 
Line 1336:         for drive in self._devices[vm.DISK_DEVICES][:]:
Line 1337:             if not hasattr(drive, 'volumeChain'):


--
To view, visit http://gerrit.ovirt.org/13396
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3291032ff85254d5c3a4260bffb7003ca3502d22
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list