On 07/20/2017 03:22 AM, Erik Skultety wrote:
@@ -594,6 +600,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) int ret = -1; virNodeDeviceObjPtr obj = NULL; virNodeDeviceDefPtr def; + char *name = NULL; + char *parent = NULL; + char *parent_wwnn = NULL; + char *parent_wwpn = NULL; + char *parent_fabric_wwn = NULL; char *wwnn = NULL, *wwpn = NULL; int parent_host = -1;
@@ -609,12 +620,24 @@ nodeDeviceDestroy(virNodeDevicePtr device) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) < 0) goto cleanup;
- /* virNodeDeviceGetParentHost will cause the device object's lock - * to be taken, so grab the object def which will have the various - * fields used to search (name, parent, parent_wwnn, parent_wwpn, - * or parent_fabric_wwn) and drop the object lock. */ + /* Because we're about to release the lock and thus run into a race + * possibility (however improbably) with a udevAddOneDevice change + * event which would essentially free the existing @def (obj->def) and + * replace it with something new, we need to save off and use the + * various fields that virNodeDeviceObjListGetParentHost will use */
And as I originally suggested I would allocate a new temporary @def structure, initialize it, pass it to the *GetParentHost method like nothing out of the ordinary happened and mentioned in the commentary why we've done it that way (which you already do in this patch).
So you'd prefer some sort of virNodeDeviceDefCopy function be created? Or use VIR_ALLOC(def) and copy in the 5 fields only to then virNodeDeviceDefFree() it afterwards?
Yeah, exactly, I'm aware of those unused extra field, I don't know it just looked more appealing and transparent to me, again, matter of preference, the way I see it, you store/pass it in a much more compact way with the added benefit of other, in this case currently unused, fields, should virNodeDeviceObjListGetParentHost ever need them.
Erik
You cut off my next line: "Seems like overkill to me." Still in actually trying to implement that - I've come to realize that @def for the CREATE path will be the only time parent_wwnn, parent_wwpn, and parent_fabric_wwn actually exist. They're not saved in the vHBA that's created... All that the created vHBA gets is the <parent>, so the delete paths do not have to call virNodeDeviceObjListGetParentHost instead they can just copy the parent_name and make a nodeDeviceObjFindByName on the parent after unlocking the original obj, then call virVHBAManageVport with the parent_host # That of course means virNodeDeviceObjListGetParentHost doesn't need the third parameter either. And I don't have to create virNodeDeviceDefCopy (although it's a benign exercise). John