Re: [libvirt] [PATCH v5 1/3] nodedev: Fix usage of virNodeDeviceObjListGetParentHost
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