@@ -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