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.