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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
