[libvirt] [PATCH v3 17/18] util: Alter virNodeDevice{Create|Delete}Vport to use nodedev APIs

[Laine Stump]

On 03/15/2017 10:22 AM, John Ferlan wrote:
> 
> 
> On 03/12/2017 09:20 PM, Laine Stump wrote:
>> On 03/10/2017 04:10 PM, John Ferlan wrote:
>>> If we have a connection pointer there's no sense walking through the
>>> sysfs in order to create/destroy the vHBA. Instead, let's make use of
>>> the node device create/destroy API's.
>>>
>>> Since we don't have to rewrite all the various parent options for
>>> the test driver in order to test whether the storage pool creation
>>> works as the node device creation has been tested already, let's just
>>> use the altered API to test the storage pool paths.
>>>
>>> Fix a "bug" in the storage pool test driver code which "assumed"
>>> testStoragePoolObjSetDefaults should fill in the configFile for
>>> both the Define/Create (persistent) and CreateXML (transient) pools
>>> by just VIR_FREE() of the pool during CreateXML.  Because the
>>> configFile was filled in, during Destroy, the pool wouldn't be
>>> free causing a test using the same name pool to fail.
>>
>> Without trying to go through this patch, the commit log makes it sound like there are 3 separate things being done. Or am I misinterpreting? Can it maybe be split up so a mindless reviewer doesn't need to do any work to figure out which is the code fixing the bug. If no splitting is possible/useful, then I'll tackle it as-is tomorrow.
>>
> 
> What's happening here is using the NodeDevice API's in order to create
> the vHBA instead of essentially repeating what the nodedev API does for
> create.
> 
> In working through the code I discovered the test_driver
> pool->configFile "bug" and it seems "over described" it... I can
> separate that.
> 
> The primary reason for doing this is I didn't want to rewrite all the
> parent* options in the test driver in order to test the storage pool XML
> options. By using the node device create - I get that for free!
> 
> I also figured it'd be "quicker" and more reliable to use what nodedev
> driver has 'in memory' rather than walking through the file system that
> theoretically udev has already done for us and nodedev has saved away.
> 
> I could really take the hard road and create an API that would handle
> all those parent* options and do the right thing. That would mean both
> nodedevice and storage pool would call that.

This ^. (or something similar). As Dan just pointed out with some
concrete reasons (and I also said in a previous review, but without
providing the good reasons like Dan did) calling a libvirt public API
from within libvirt may be tempting but not a good idea.