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

[John Ferlan]

On 03/15/2017 10:46 AM, Daniel P. Berrange wrote:
> 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.
> 

OK - so I'll take a different tack with this...

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

I think I originally got the idea from qemuMigrationPrecreateDisk and a
desire (at the time) to not create an API for perhaps some less obvious
to some backporting reasons.  Also since NodeDeviceCreateXML essentially
has a single purpose - I guess it just felt "bounded"...


John

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