On 02/27/2017 01:15 PM, Laine Stump wrote:
> On 02/20/2017 08:18 AM, 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.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/conf/node_device_conf.c | 121
>> ++++++++++++++++++++++++++++++++++++++++++++
>> src/test/test_driver.c | 6 +++
>> tests/fchosttest.c | 15 ++++++
>> 3 files changed, 142 insertions(+)
>>
>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>> index 68ed5ad..1d76096 100644
>> --- a/src/conf/node_device_conf.c
>> +++ b/src/conf/node_device_conf.c
>> @@ -2390,6 +2390,82 @@ nodeDeviceCheckParent(virConnectPtr conn,
>> /**
>> * @conn: Connection pointer
>> + * @fchost: Pointer to the vHBA adapter
>> + *
>> + * If we have a valid connection, then use the node device create
>> + * XML API rather than traversing through the sysfs to create the vHBA.
>> + * Generate the Node Device XML using the Storage vHBA Adapter providing
>> + * either the parent name, parent wwnn/wwpn, or parent fabric_name if
>> + * available to the Node Device code. Since the Storage XML processing
>> + * requires the wwnn/wwpn to be used for the vHBA to be provided (and
>> + * not generated), we can use that as the fc_host wwnn/wwpn. This will
>> + * allow for easier search later when we need it.
>> + *
>> + * Returns vHBA name on success, NULL on failure with an error
>> message set
>> + */
>> +static char *
>> +nodeDeviceCreateNodeDeviceVport(virConnectPtr conn,
>> + virStorageAdapterFCHostPtr fchost)
>> +{
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> + char *vhbaStr = NULL;
>> + virNodeDevicePtr dev = NULL;
>> + char *name;
>> + bool created = false;
>> +
>> + /* If the nodedev already knows about this vHBA, then we're not
>> + * managing it - we'll just use it. */
>> + if ((dev = virNodeDeviceLookupSCSIHostByWWN(conn, fchost->wwnn,
>> + fchost->wwpn, 0)))
>
> I get nervous anytime I see a call to a toplevel public libvirt API
> inside some low level function even when it's in one of the driver
> directories. But this is in the conf directory. Is there a precedent for
> doing that? It doesn't seem like a good idea - if it's called from
> within any other nodedev function it could end up in a deadlock of the
> driver-wide lock.
I usually include that type of tidbit in the commit message too...
One short answer is storage_conf calls virNodeDeviceGetParentName
As for the deadlock concern... I have a fix for that ;-), but it's one
long series to review... Look for my virPoolObj[Table]Ptr series. It
needs an update, but those driver hammer locks get removed... They're
replaced by a locking system similar to qemu_driver.
>
> Is there some other organization that could make this cleaner? (haven't
> thought about what yet)
>
>
I'm OK with suggestions that don't need to rototill too much code.
The "idea" to generate the XML on the fly was from how volumes are
migrated in qemuMigrationPrecreateDisk. Since that was possible and I
knew (proved by testing too) that the nodedev creation by XML was
possible - that's what I figured would make all this code a bit more
"organized".
Additionally all this used to be part of virvhba.c (hence the util: in
the original commit message), but I had to move it to node_device_conf
because of the aforementioned rule of not including a conf file in util.
Another goal of this series was to not rely on perusing the file system
to create a vHBA - instead one would think it would be much quicker to
ask nodedev since it should already have the answer. I think it would be
good to not snake through directory trees, but rather rely on nodedev to
provide the answer.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
