[libvirt] [PATCH v5 1/3] nodedev: Fix usage of virNodeDeviceObjListGetParentHost

[John Ferlan]

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