[libvirt] [PATCH 2/8] tests: Add createVHBAByStoragePool-by-parent to fchosttest

[Michal Privoznik]

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 at 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