Dan Kenigsberg has posted comments on this change.
Change subject: Cisco VM-FEX support vdsm hooks
......................................................................
Patch Set 13: I would prefer that you didn't submit this
(3 inline comments)
....................................................
File vdsm_hooks/vmfex/before_vm_migrate_destination.py
Line 71: def createDirectPool(conn):
Line 72: with open('/var/run/vdsm/hook-vmfex.lock', 'w') as f:
Line 73: fcntl.flock(f.fileno(), fcntl.LOCK_EX)
Line 74: try:
Line 75: #singleton - doublecheck after taking lock
I'm a bit puzzled by this comment and this code.. I would prefer if you called
removeDirectPool() (and drop the locking there if it is a problem)
Line 76: #to avoid a race between two threads
Line 77: if 'direct-pool' in conn.listNetworks():
Line 78: dpool = conn.networkLookupByName('direct-pool')
Line 79: #destroy and undefine direct-pool
Line 132: return
Line 133: #Now we know for sure that the pool exists...
Line 134: #are there VMs running, and are they using 802.1Qbh?
Line 135: #if yes, can't touch the existing pool
Line 136: if not qbhInUse(conn):
there's still a race hiding here: a VM using qbh may be started right after
this check.
I believe we should take the lock outside this function :-(
Line 137: #no VMs use the existing pool, it can be checked and updated
Line 138: #is the pool the same as list of available dNICs?
Line 139: if not compareDPoolToUsableNics(conn):
Line 140: #not the same, something changed, recreating
....................................................
File vdsm_hooks/vmfex/before_vm_start.py
Line 105: def createDirectPool(conn):
Line 106: with open('/var/run/vdsm/hook-vmfex.lock', 'w') as f:
Line 107: fcntl.flock(f.fileno(), fcntl.LOCK_EX)
Line 108: try:
Line 109: #singleton - doublecheck after taking lock
isn't it lovely to repeat each change twice?
Line 110: #to avoid a race between two threads
Line 111: if 'direct-pool' in conn.listNetworks():
Line 112: dpool = conn.networkLookupByName('direct-pool')
Line 113: #destroy and undefine direct-pool
--
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: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Yasny <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Dan Yasny <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
