Change in vdsm[master]: vmfex hook: hold a ref for a virDomain object

fromani at redhat.com fromani at redhat.com
Mon Feb 8 08:03:37 UTC 2016


Francesco Romani has posted comments on this change.

Change subject: vmfex hook: hold a ref for a virDomain object
......................................................................


Patch Set 1: Code-Review+1

(4 comments)

I think this patch in itself is good enough, but we surely need more work on the weakmethod side.
So, +1 for this patch.

https://gerrit.ovirt.org/#/c/53160/1//COMMIT_MSG
Commit Message:

Line 10: refernce circles.
typos: reference cycles


Line 10: This causes a surprising behaviour: if we do not hold
       : a reference to virdomain.Notifying, the underlying virDomain is
       : garbage-collected before being accessed for the first time.
this is indeed surprising and alarming. Do you have a reproducer script I can play with it? I though a bit about this behaviour but I can't yet seen an obvious cause.


Line 12: garbage-collected before being accessed for the first time.
Line 13: 
Line 14: This patch quickly holds a named reference to the domain. I did not find
Line 15: other pieces of code with the same problem, however, the mechanism seems
Line 16: dangerously error-prone.
indeed, we need a followup-fix at very least for 3.6 and for master
Line 17: 
Line 18: Change-Id: Ida7488db5f8af51da5a2931d8b8a6c51d99c36f0
Line 19: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1305338


https://gerrit.ovirt.org/#/c/53160/1/vdsm_hooks/vmfex_dev/vmfex_vnic.py
File vdsm_hooks/vmfex_dev/vmfex_vnic.py:

Line 81: def qbhInUse(conn):
Line 82:     """Returns whether there is already some VM with a 802.1Qbh (most likely to
Line 83:     be vmfex)."""
Line 84:     for vmid in conn.listDomainsID():
Line 85:         domobj = conn.lookupByID(vmid)
this looks nicer, but as stated in the commit message is very alarming that the compact form was error-prone.
Maybe worth a comment (FIXME? TODO?) until we have safer behaviour on weakmethod()
Line 86:         domxml = minidom.parseString(domobj.XMLDesc(0))
Line 87:         for vport in domxml.getElementsByTagName('virtualport'):
Line 88:             if vport.getAttribute('type') == '802.1Qbh':
Line 89:                 return True


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida7488db5f8af51da5a2931d8b8a6c51d99c36f0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Anonymous Coward #1001177
Gerrit-Reviewer: Dan Yasny <dyasny at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list