[libvirt] [PATCH v3 17/18] util: Alter virNodeDevice{Create|Delete}Vport to use nodedev APIs

Daniel P. Berrange berrange at redhat.com
Wed Mar 15 14:46:15 UTC 2017


On Fri, Mar 10, 2017 at 04:10:49PM -0500, 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.

In general we should *not* call out to the public API entry points,
from inside libvirt code. Instead we should have an internal only
function that is called from both the public API entry point, and
from whatever other context needs the same functionality.

Calling the public API entry points directly imposes access control
checks on the internal code which is not good. Also if public APIs
are triggered in clean up code paths, then it'll blow away any
reported error.

> 
> 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 66cb78d..0c25f58 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -1916,6 +1916,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)))
> +        goto skip_create;
> +
> +    virBufferAddLit(&buf, "<device>\n");
> +    virBufferAdjustIndent(&buf, 2);
> +    if (fchost->parent)
> +        virBufferEscapeString(&buf, "<parent>%s</parent>\n",
> +                              fchost->parent);
> +    else if (fchost->parent_wwnn && fchost->parent_wwpn)
> +        virBufferAsprintf(&buf, "<parent wwnn='%s' wwpn='%s'/>\n",
> +                          fchost->parent_wwnn, fchost->parent_wwpn);
> +    else if (fchost->parent_fabric_wwn)
> +        virBufferAsprintf(&buf, "<parent fabric_wnn='%s'/>\n",
> +                          fchost->parent_fabric_wwn);
> +    virBufferAddLit(&buf, "<capability type='scsi_host'>\n");
> +    virBufferAdjustIndent(&buf, 2);
> +    virBufferAsprintf(&buf, "<capability type='fc_host' wwnn='%s' wwpn='%s'>\n",
> +                      fchost->wwnn, fchost->wwpn);
> +    virBufferAddLit(&buf, "</capability>\n");
> +    virBufferAdjustIndent(&buf, -2);
> +    virBufferAddLit(&buf, "</capability>\n");
> +    virBufferAdjustIndent(&buf, -2);
> +    virBufferAddLit(&buf, "</device>\n");

We really shouldn't be generating XML internally just to immediately give
back to ourselves. We should populate the appropriate internal config data
struct instead and avoid XML entirely.

> +
> +    if (!(vhbaStr = virBufferContentAndReset(&buf))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("unable to create node device XML"));
> +        goto cleanup;
> +    }
> +
> +    if (!(dev = virNodeDeviceCreateXML(conn, vhbaStr, 0)))
> +        goto cleanup;
> +    created = true;
> +
> + skip_create:
> +    if (VIR_STRDUP(name, virNodeDeviceGetName(dev)) < 0) {
> +        /* If we created, then destroy it */
> +        if (created)
> +            ignore_value(virNodeDeviceDestroy(dev));
> +    }
> +
> + cleanup:
> +    VIR_FREE(vhbaStr);
> +    virObjectUnref(dev);
> +    return name;
> +}
> +

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|




More information about the libvir-list mailing list