On 27.02.2017 17:50, John Ferlan wrote:
>
>
> On 02/27/2017 10:36 AM, Michal Privoznik wrote:
>> On 20.02.2017 14:18, John Ferlan wrote:
>>> Add a new test to fchosttest in order to test creation of our vHBA
>>> via the Storage Pool logic. Unlike the real code, we cannot yet use
>>> the virVHBA* API's because they (currently) traverse the file system
>>> in order to get the parent vport capable scsi_host. Besides there's
>>> no "real" NPIV device here - so we have to take some liberties, at
>>> least for now.
>>>
>>> Instead, we'll follow the node device tests partially in order to
>>> create and destroy the vHBA with the test node devices.
>>>
>>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>>> ---
>>> src/test/test_driver.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>> tests/fchosttest.c | 64 ++++++++++++++++++++++++++++++++++
>>> 2 files changed, 157 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index 5fef3f1..4dff0f1 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -4365,6 +4365,32 @@ testConnectFindStoragePoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
>>> }
>>>
>>>
>>> +static virNodeDeviceDefPtr
>>> +testNodeDeviceMockCreateVport(virConnectPtr conn,
>>> + const char *wwnn,
>>> + const char *wwpn);
>>> +static int
>>> +testCreateVport(virConnectPtr conn,
>>> + const char *wwnn,
>>> + const char *wwpn)
>>> +{
>>> + /* The storage_backend_scsi createVport() will use the input adapter
>>> + * fields parent name, parent_wwnn/parent_wwpn, or parent_fabric_wwn
>>> + * in order to determine whether the provided parent can be used to
>>> + * create a vHBA or will find "an available vport capable" to create
>>> + * a vHBA. In order to do this, it uses the virVHBA* API's which traverse
>>> + * the sysfs looking at various fields (rather than going via nodedev).
>>> + *
>>> + * Since the test environ doesn't have the sysfs for the storage pool
>>> + * test, at least for now use the node device test infrastructure to
>>> + * create the vHBA. In the long run the result is the same. */
>>> + if (!testNodeDeviceMockCreateVport(conn, wwnn, wwpn))
>>> + return -1;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +
>>> static virStoragePoolPtr
>>> testStoragePoolCreateXML(virConnectPtr conn,
>>> const char *xml,
>>> @@ -4395,6 +4421,24 @@ testStoragePoolCreateXML(virConnectPtr conn,
>>> goto cleanup;
>>> def = NULL;
>>>
>>> + if (pool->def->source.adapter.type ==
>>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>>> + /* In the real code, we'd call virVHBAManageVport followed by
>>> + * find_new_device, but we cannot do that here since we're not
>>> + * mocking udev. The mock routine will copy an existing vHBA and
>>> + * rename a few fields to mock that. So in order to allow that to
>>> + * work properly, we need to drop our lock */
>>> + testDriverUnlock(privconn);
>>> + if (testCreateVport(conn, pool->def->source.adapter.data.fchost.wwnn,
>>> + pool->def->source.adapter.data.fchost.wwpn) < 0) {
>>> + virStoragePoolObjRemove(&privconn->pools, pool);
>>> + pool = NULL;
>>> + testDriverLock(privconn);
>>> + goto cleanup;
>>> + }
>>> + testDriverLock(privconn);
>>
>> So we need this testDriverLock() and Unlock() calls because
>> testCreateVport() calls testNodeDeviceMockCreateVport() which then call
>> top level APIs for looking up a nodedev and fetching its XML. Pardon my
>> language but that looks stup^Wweird. Mind fixing that?
>
> Well it does follow similar ugliness in testNodeDeviceCreateXML
>
> Somewhat ironically I have an RFC series posted that can reduce/remove
> the need a well, but it's quite a bit more change... It also modifies
> nodedev's to use hash tables instead of (long) linked lists that are
> currently being used. With that series in place, this ugliness wouldn't
> be needed.
>
> Anyway, so as an adjustment at least here... I could move the hunk below
> the pool->active = 1 and before the event. Then drop the lock entirely
> before call the testCreateVport. Would also need to add a 'isLocked' so
> that the unlock isn't called unnecessarily. Of course that's almost as
> equally as ugly.
>
> Unless you had another methodology in mind...
What about these lines (in testNodeDeviceMockCreateVport):
if (!(objcopy = virNodeDeviceFindByName(&driver->devs, "scsi_host11")) ||
!(xml = virNodeDeviceDefFormat(objcopy->def)) ||
!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
goto cleanup;
I haven't even compile-tested them, but in general - they call the important parts of the public APIs without us needing to lock()/unlock() the driver meanwhile. There might be some additional unlock(objcopy) required, unref() or free(), but I'm willing to do that if it saves us from weird driver lock()/unlock() calls.
If you have this ^^ patch before this one, you can just drop test driver lock()/unlock() here.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
