Re: [libvirt] [PATCH 1/1] Implement NPIV

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

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