[libvirt] [PATCH 6/8] util: Alter virNodeDevice{Create|Delete}Vport to use nodedev APIs

Laine Stump laine at laine.org
Mon Feb 27 18:15:54 UTC 2017


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 at redhat.com>
> ---
>   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.

Is there some other organization that could make this cleaner? (haven't 
thought about what yet)





More information about the libvir-list mailing list