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@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 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
