Change in vdsm[master]: Cisco VM-FEX support vdsm hooks

danken at redhat.com danken at redhat.com
Wed Oct 17 07:07:05 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: Cisco VM-FEX support vdsm hooks
......................................................................


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

(5 inline comments)

I have only one major question, about code repetition in handleDirectPool().

....................................................
File vdsm_hooks/vmfex/before_vm_migrate_destination.py
Line 8:     import libvirtconnection
Line 9: except ImportError:
Line 10:     #3.1 compat
Line 11:     from vdsm import libvirtconnection
Line 12: import fcntl
minor: standard modules should be imported first
Line 13: from xml.dom import minidom
Line 14: 
Line 15: '''
Line 16: Placed in before_vm_migrate_destination


....................................................
File vdsm_hooks/vmfex/before_vm_start.py
Line 92:     # Example ['eth0','eth1']
Line 93:     nics = []
Line 94:     for root, dirs, names in os.walk('/sys/devices/'):
Line 95:         if 'address' in names and 'pci' in root:
Line 96:             f = open(root + '/address', 'r')
with file(root + '/address', 'r') as f:
  mac = f.readlines()[0].strip()

would have been slightly nicer
Line 97:             mac = f.readlines()[0].strip()
Line 98:             f.close()
Line 99:             if mac == '00:00:00:00:00:00':
Line 100:                 eth = root.split('/')[-1]


Line 148: def compareDPoolToUsableNics(conn):
Line 149:     #return True if currently defined direct-pool matches the set
Line 150:     #returned by getUsableNics()
Line 151:     dpool = conn.networkLookupByName('direct-pool')
Line 152:     definedNics = []
minor: this could be a set() to begin with.
Line 153:     dpoolxml = minidom.parseString(dpool.XMLDesc(0))
Line 154:     for iface in dpoolxml.getElementsByTagName('interface'):
Line 155:         definedNics.append(iface.getAttribute('dev'))
Line 156:     if set(definedNics) == set(getUsableNics()):


Line 158:     else:
Line 159:         return False
Line 160: 
Line 161: 
Line 162: def handleDirectPool(conn):
minor: this is a very vague name. I think that updateDirectPool is clearer.
Line 163:     #TODO: take this part and everything it uses out and into a separate
Line 164:     # module
Line 165: 
Line 166:     #is direct-pool defined? If not, just create it and move on


Line 168:         createDirectPool(conn)
Line 169:         return
Line 170:     #Now we know for sure that the pool exists...
Line 171:     #are there VMs running?
Line 172:     if conn.listDomainsID():
if there is no Domain running, qbhInUse() would return False.
so this block can be simplified to

	        if not qbhInUse(conn):
	        #no VMs use the existing pool, it can be checked and updated
	            #is the pool the same as list of available dNICs?
	            if not compareDPoolToUsableNics(conn):
	            #not the same, something changed, recreating
	                removeDirectPool(conn)
	                createDirectPool(conn)

eliminating the repeated code. Am I right?
Line 173:         #does any of the running VMs havd a Qbh nic?
Line 174:         #if yes, can't touch the existing pool
Line 175:         if not qbhInUse(conn):
Line 176:         #no VMs use the existing pool, it can be checked and updated


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45a7fa46919bb39a648dff190c40618395990e91
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Yasny <dyasny at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dan Yasny <dyasny at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list