[libvirt] [PATCH 1/1] Implement NPIV
Dave Allan
dallan at redhat.com
Wed Apr 8 21:53:57 UTC 2009
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
More information about the libvir-list
mailing list