11:28 a.m.
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(a)redhat.com>
>> ---
>> 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