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

[John Ferlan]

>>
>> FWIW: The lock gets easier with RFC series and of course that's in the
>> back of my mind every time I touch this code...  All the fun I'll have
>> merging changes...
>>
> 
> Ah, which patches are that? I want to review them.
> 
> Michal
> 

I need to post an update since nodedev and storage changes have caused
merge conflicts, but the initial version is:

http://www.redhat.com/archives/libvir-list/2017-February/msg00519.html

Essentially for nodedev, interface, nwfilter, secret, volume, storage,
and network I've attempted to commonalize the hash table logic from
domain (and domain snapshot). Each of the drivers ends up then having a
very common look and feel. Unfortunately they're not simple to look at.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Michal Privoznik]

On 02/27/2017 11:05 PM, John Ferlan wrote:
> [...]
> 
>>> 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(>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
>>
> 
> The above doesn't work as cleanly as one would hope as eventtest hangs,
> but after a bit of finagling, the following works:

Ah, now I see why it hangs. The problem lies elsewhere:
testNodeDeviceCreateXML() calls public APIs thus it unlocks the driver
again. I mean, testNodeDeviceMockCreateVport() is not the only function
it calls with unlocked driver. Le sigh.

> 
> 
> if (!(objcpy = virNodeDeviceFindByName(>devs, "scsi_host11")))
> goto cleanup;
> 
> xml = virNodeDeviceDefFormat(objcpy->def);
> virNodeDeviceObjUnlock(objcpy);
> if (!xml)
> goto cleanup;
> 
> if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
> goto cleanup;

Yup, I've expected that we will need to unlock the object returned by
virNodeDeviceFindByName(). This looks much nicer IMO. But we still need
to fix testNodeDeviceCreateXML(). Working on the fix now.

> 
> Going this route also removes the need for the existing caller to do
> unlock/lock game as well.
> 
> John
> 
> FWIW: The lock gets easier with RFC series and of course that's in the
> back of my mind every time I touch this code...  All the fun I'll have
> merging changes...
> 

Ah, which patches are that? I want to review them.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[John Ferlan]

[...]

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

The above doesn't work as cleanly as one would hope as eventtest hangs,
but after a bit of finagling, the following works:


if (!(objcpy = virNodeDeviceFindByName(>devs, "scsi_host11")))
goto cleanup;

xml = virNodeDeviceDefFormat(objcpy->def);
virNodeDeviceObjUnlock(objcpy);
if (!xml)
goto cleanup;

if (!(def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL)))
goto cleanup;

Going this route also removes the need for the existing caller to do
unlock/lock game as well.

John

FWIW: The lock gets easier with RFC series and of course that's in the
back of my mind every time I touch this code...  All the fun I'll have
merging changes...

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [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 
>>> ---
>>>  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(>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(>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 

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

[John Ferlan]

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 
>> ---
>>  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(>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...

John


> 
>> +}
>> +
> 
> Otherwise looking good.
> 
> Michal
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Michal Privoznik]

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 
> ---
>  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(>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?

> +}
> +

Otherwise looking good.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[John Ferlan]

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 
---
 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(>pools, pool);
+pool = NULL;
+testDriverLock(privconn);
+goto cleanup;
+}
+testDriverLock(privconn);
+}
+
 if (testStoragePoolObjSetDefaults(pool) == -1) {
 virStoragePoolObjRemove(>pools, pool);
 pool = NULL;
@@ -4405,7 +4449,6 @@ testStoragePoolCreateXML(virConnectPtr conn,
 event = virStoragePoolEventLifecycleNew(pool->def->name, pool->def->uuid,
 VIR_STORAGE_POOL_EVENT_STARTED,
 0);
-
 ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid,
 NULL, NULL);
 
@@ -4539,6 +4582,44 @@ testStoragePoolBuild(virStoragePoolPtr pool,
 
 
 static int
+testDestroyVport(testDriverPtr privconn,
+ const char *wwnn ATTRIBUTE_UNUSED,
+ const char *wwpn ATTRIBUTE_UNUSED)
+{
+int ret = -1;
+virNodeDeviceObjPtr obj = NULL;
+virObjectEventPtr event = NULL;
+
+/* NB: Cannot use virVHBAGetHostByWWN (yet) like the storage_backend_scsi
+ * deleteVport() helper since that traverses the file system looking for
+ * the wwnn/wwpn. So our choice short term is to cheat and use the name
+ * (scsi_host12) we know was created.
+ *
+ * Reaching across the boundaries of space and time into the
+ * Node Device in order to remove */
+if (!(obj = virNodeDeviceFindByName(>devs, "scsi_host12"))) {
+virReportError(VIR_ERR_NO_NODE_DEVICE, "%s",
+   _("no node device with matching name 'scsi_host12'"));
+goto cleanup;
+}
+
+event = virNodeDeviceEventLifecycleNew("scsi_host12",
+   VIR_NODE_DEVICE_EVENT_DELETED,
+   0);
+
+virNodeDeviceObjRemove(>devs, );
+
+ret = 0;
+
+ cleanup:
+if (obj)
+virNodeDeviceObjUnlock(obj);
+testObjectEventQueue(privconn, event);
+return ret;
+}
+
+
+static int