[libvirt] [PATCH 1/1] Implement NPIV

[Daniel P. Berrange]

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;

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|