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