Daniel P. Berrange wrote:

On Tue, Apr 07, 2009 at 05:16:31PM -0400, David Allan wrote:

...

static int +virStorageBackendVportCreateDelete(virConnectPtr conn, + virStoragePoolObjPtr pool, + int operation) +{ + int fd = -1; + int retval = 0; + char *operation_path; + const char *operation_file; + char *vport_name; + size_t towrite = 0; + + switch (operation) { + case VPORT_CREATE: + operation_file = LINUX_SYSFS_VPORT_CREATE_POSTFIX; + break; + case VPORT_DELETE: + operation_file = LINUX_SYSFS_VPORT_DELETE_POSTFIX; + break; + default: + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("Invalid vport operation (%d)"), operation); + retval = -1; + goto no_unwind; + break; + } + + if (virAsprintf(&operation_path, + "%s/%s/%s", + LINUX_SYSFS_FC_HOST_PREFIX, + pool->def->source.adapter, + operation_file) < 0) { + + virReportOOMError(conn); + retval = -1; + goto no_unwind; + } + + VIR_DEBUG(_("Vport operation path is '%s'"), operation_path); + + fd = open(operation_path, O_WRONLY); + + if (fd < 0) { + virReportSystemError(conn, errno, + _("Could not open '%s' for vport operation"), + operation_path); + retval = -1; + goto free_path; + } + + if (virAsprintf(&vport_name, + "%s:%s", + pool->def->source.wwpn, + pool->def->source.wwnn) < 0) { + + virReportOOMError(conn); + retval = -1; + goto close_fd; + } + + towrite = sizeof(vport_name); + if (safewrite(fd, vport_name, towrite) != towrite) { + virReportSystemError(conn, errno, + _("Write of '%s' to '%s' during " + "vport create/delete failed"), + vport_name, operation_path); + retval = -1; + }

This chunk of code could be a little simplified by making it call out to virFileWriteStr() from src/util.h

Thanks for the pointer--I'll make that change.

...

+ + VIR_FREE(vport_name); +close_fd: + close(fd); +free_path: + VIR_FREE(operation_path); +no_unwind: + VIR_DEBUG("%s", _("Vport operation complete")); + return retval; +} + + +static int +virStorageBackendSCSIStartPool(virConnectPtr conn, + virStoragePoolObjPtr pool) +{ + int retval = 0; + + if (pool->def->source.wwnn != NULL) { + retval = virStorageBackendVportCreateDelete(conn, pool, VPORT_CREATE); + } + + return retval; +} + + +static int +virStorageBackendSCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED) +{ + int retval = 0; + + if (pool->def->source.wwnn != NULL) { + retval = virStorageBackendVportCreateDelete(conn, pool, VPORT_DELETE); + } + + return retval; +} + + +static int virStorageBackendSCSITriggerRescan(virConnectPtr conn, uint32_t host) { int fd = -1; int retval = 0; char *path; + size_t towrite = 0;

VIR_DEBUG(_("Triggering rescan of host %d"), host);

@@ -593,9 +702,8 @@ virStorageBackendSCSITriggerRescan(virConnectPtr conn, goto free_path; }

- if (safewrite(fd, - LINUX_SYSFS_SCSI_HOST_SCAN_STRING, - sizeof(LINUX_SYSFS_SCSI_HOST_SCAN_STRING)) < 0) { + towrite = sizeof(LINUX_SYSFS_SCSI_HOST_SCAN_STRING); + if (safewrite(fd, LINUX_SYSFS_SCSI_HOST_SCAN_STRING, towrite) != towrite) {

virReportSystemError(conn, errno, _("Write to '%s' to trigger host scan failed"), @@ -645,5 +753,7 @@ out: virStorageBackend virStorageBackendSCSI = { .type = VIR_STORAGE_POOL_SCSI,

+ .startPool = virStorageBackendSCSIStartPool, .refreshPool = virStorageBackendSCSIRefreshPool, + .stopPool = virStorageBackendSCSIStopPool,

As per the previous mail, I think these are better switched over to the .buildPool and .deletePool drivers.

...

diff --git a/src/storage_conf.c b/src/storage_conf.c index 9e25ccb..4827d69 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -42,6 +42,7 @@ #include "buf.h" #include "util.h" #include "memory.h" +#include "logging.h"

#define VIR_FROM_THIS VIR_FROM_STORAGE

@@ -451,6 +452,55 @@ error: }

+static int +getNPIVParameters(virConnectPtr conn, + virStoragePoolDefPtr pool, + xmlXPathContextPtr ctxt) +{ + int retval = 0; + + if ((pool->source.wwpn = virXPathString(conn, + "string(/pool/source/adapter/@wwpn)", + ctxt)) == NULL) { + VIR_DEBUG("%s", _("No WWPN found")); + goto out; + } + + VIR_DEBUG(_("Found WWPN '%s'"), pool->source.wwpn); + + if ((pool->source.wwnn = virXPathString(conn, + "string(/pool/source/adapter/@wwnn)", + ctxt)) == NULL) { + VIR_DEBUG("%s", _("No WWNN found")); + goto out; + } + + VIR_DEBUG(_("Found WWNN '%s'"), pool->source.wwnn); + + if (pool->source.wwpn != NULL || pool->source.wwnn != NULL) { + if (pool->source.wwpn == NULL || pool->source.wwnn == NULL) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + _("Both WWPN and WWNN must be specified " + "if either is specified")); + retval = -1; + goto cleanup; + } + } + + /* We don't check the values for validity, because the kernel does + * that. Any invalid values will be rejected when the pool + * starts. The kernel has the final say on what it will accept + * and we should not second guess it. */ + +cleanup: + VIR_FREE(pool->source.wwpn); + VIR_FREE(pool->source.wwnn); + +out: + return retval; +} + + static virStoragePoolDefPtr virStoragePoolDefParseDoc(virConnectPtr conn, xmlXPathContextPtr ctxt, @@ -590,6 +640,9 @@ virStoragePoolDefParseDoc(virConnectPtr conn, "%s", _("missing storage pool source adapter name")); goto cleanup; } + if (getNPIVParameters(conn, ret, ctxt) < 0) { + goto cleanup; + } }

authType = virXPathString(conn, "string(/pool/source/auth/@type)", ctxt); diff --git a/src/storage_conf.h b/src/storage_conf.h index 4e35ccb..cfd8b14 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -192,6 +192,12 @@ struct _virStoragePoolSource { /* Or an adapter */ char *adapter;

+ /* And an optional WWPN */ + char *wwpn; + + /* And an optional WWNN */ + char *wwnn; +

Since adapter, wwpn and wwnn are all associated with each other, I think we should put them all into a union here. eg, replace the current

char *adapter;

with

union { char *name; char *wwpn; char *wwnn; } adapter;

I don't have completely working code yet, but here's what I'm thinking: We can't use a union because wwpn and wwnn will always be in use at the same time, but I agree that we're starting to collect a bunch of related information about adapters that clutters up the pool source struct. I've created a struct along the lines of virStoragePoolSourceHost to contain this information. It will need some additional fields. /* * For adapter based pools, info on the relevant adapter */ typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter; typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr; struct _virStoragePoolSourceAdapter { char *name; char *wwpn; char *wwnn; }; Since we might be creating a virtual adapter on a physical adapter that we specify by wwpn/wwnn, we'll need two places to specify wwpn/wwnn in the XML. I propose: <pool type="scsi"> <name>npiv</name> <source> <adapter name="host6" wwpn="0000111122223333" wwnn="4444555566667777"/> <vport wwpn="88889999aaaabbbb" wwnn="ccccddddeeeeffff"/> </source> <target> <path>/dev/disk/by-id</path> </target> </pool> Dave

Daniel P. Berrange wrote:

On Wed, Apr 08, 2009 at 05:53:57PM -0400, Dave Allan wrote:

...

Daniel P. Berrange wrote:

...

...

diff --git a/src/storage_conf.h b/src/storage_conf.h index 4e35ccb..cfd8b14 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -192,6 +192,12 @@ struct _virStoragePoolSource { /* Or an adapter */ char *adapter;

+ /* And an optional WWPN */ + char *wwpn; + + /* And an optional WWNN */ + char *wwnn; + Since adapter, wwpn and wwnn are all associated with each other, I think we should put them all into a union here. eg, replace the current

char *adapter;

with

union { char *name; char *wwpn; char *wwnn; } adapter; I don't have completely working code yet, but here's what I'm thinking:

We can't use a union because wwpn and wwnn will always be in use at the same time, but I agree that we're starting to collect a bunch of related information about adapters that clutters up the pool source struct. I've created a struct along the lines of virStoragePoolSourceHost to contain this information. It will need some additional fields.

I don't know what I was thinking when I wrote 'union'. I absolutely meant an anonymous 'struct'. A named struct as you show below is even better

...

/*

* For adapter based pools, info on the relevant adapter

*/ typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter; typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr; struct _virStoragePoolSourceAdapter { char *name; char *wwpn; char *wwnn; };

Since we might be creating a virtual adapter on a physical adapter that we specify by wwpn/wwnn, we'll need two places to specify wwpn/wwnn in the XML. I propose:

Hmm, yes, this is what made me think originally that the storage pool may not be the right place to put the creation/deletion of the vport.

Working through the steps for using NPIV in our APIs...

- list of HBAs == virNodeListDevices() with cap='scsi_host'

- For each HBA - Query XML of the virNodeDevicePtr object - Look for 'vport' capability flag

You now have an virNodeDevicePtr object which you know is able to create/delete vports, lets say its name is 'host3'.

- Define config for a new vport, giving <parent> as the name of virNodeDevicePtr we just obtained

<device> <parent>host3</parent> <capability type='scsi_host'> <capability type='fc'> <wwpn>88889999aaaabbbb</wwpn> <wwnn>4444555566667777</wwnn> </capability> </capability> </device>

- Create the device with virNodeDeviceCreate(char *xml);

- Use the device by defining a SCSI storage pool using the <host></host> property of the device we just created

This keeps the notion of hierarchy / parent+child relationship between HBAs just witin the node device APIs, and avoids replicating it in the storage APIs.

The above generally sounds like a good idea to me. I am in the process of working out the specifics, but it's a much cleaner fit than trying to make this functionality go into pools.

I still think it would be worth adding a 'wwpn' and 'wwnn' to the SCSI storage pool XML config - to be used as an alternative to the 'host' parameter, since wwpn+wwnn are guarenteed stable across machines, whereas 'host' is only unique within scope of a single machine and even changes across boot.

That sounds good; a user can create the adapter with the device API and then be able to find the associated pool easily via wwpn/wwnn.

This does of course mean swe need to add new virNodeDeviceCreate and virNodeDeviceDestroy APIs, but I think it is worth doing that and they can be useful for other areas too. For example, it would allow us to have a means to create loopback devices, or create NBD devices (via qemu-nbd), which are useful things todo if you want to get into the innards of file based disks

Agreed.

...

<pool type="scsi"> <name>npiv</name> <source> <adapter name="host6" wwpn="0000111122223333" wwnn="4444555566667777"/> <vport wwpn="88889999aaaabbbb" wwnn="ccccddddeeeeffff"/>

So, we'd keep the <adapter> line as you show, but would not need the <vport> line, because the <adater> line would refer to the wwpn/wwn/host of the vport itself

That works for me.

...

</source> <target> <path>/dev/disk/by-id</path> </target> </pool>

Dave