[libvirt] [PATCH 1/1] Backend of node device API NPIV support

[Daniel P. Berrange]

On Mon, May 04, 2009 at 04:58:21PM -0400, David Allan wrote:
> @@ -258,6 +307,310 @@ cleanup:
>  }
>  
>  
> +static int
> +nodeDeviceVportCreateDelete(virConnectPtr conn,
> +                            const int parent_host,
> +                            const char *wwpn,
> +                            const char *wwnn,
> +                            int operation)
> +{
> +    int fd = -1;
> +    int retval = 0;
> +    char *operation_path;
> +    const char *operation_file;
> +    char *vport_name;
> +    size_t towrite = 0;
> +    unsigned int written = 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:
> +        virNodeDeviceReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                                 _("Invalid vport operation (%d)"), operation);
> +        retval = -1;
> +        goto no_unwind;
> +        break;
> +    }
> +
> +    if (virAsprintf(&operation_path,
> +                    "%shost%d%s",
> +                    LINUX_SYSFS_FC_HOST_PREFIX,
> +                    parent_host,
> +                    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",
> +                    wwpn,
> +                    wwnn) < 0) {
> +
> +        virReportOOMError(conn);
> +        retval = -1;
> +        goto close_fd;
> +    }
> +
> +    towrite = strlen(vport_name);
> +    written = safewrite(fd, vport_name, towrite);
> +    if (written != towrite) {
> +        virReportSystemError(conn, errno,
> +                             _("Write of '%s' to '%s' during "
> +                               "vport create/delete failed "
> +                               "(towrite: %lu written: %d)"),
> +                             vport_name, operation_path,
> +                             towrite, written);
> +        retval = -1;
> +    }
> +
> +    VIR_FREE(vport_name);
> +close_fd:
> +    close(fd);
> +free_path:
> +    VIR_FREE(operation_path);
> +no_unwind:
> +    VIR_DEBUG("%s", _("Vport operation complete"));
> +    return retval;
> +}

I think it is a little overkill to have so many cleanup points.
VIR_FREE is quite happy to get a NULL pointer, just being a no-op
in this case. So if you make sure initialize the char * strins
to NULL, you can just have

cleanup:
     VIR_FREE(vport_name);
     if (fd != -1) close(fd);
     VIR_FREE(operation_path);
     VIR_DEBUG("%s", _("Vport operation complete"));


> +
> +
> +static int
> +get_wwns(virConnectPtr conn,
> +         virNodeDeviceDefPtr def,
> +         char **wwnn,
> +         char **wwpn)
> +{
> +    virNodeDevCapsDefPtr cap = NULL;
> +    int ret = 0;
> +
> +    cap = def->caps;
> +    while (cap != NULL) {
> +        if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST &&
> +            cap->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> +            *wwnn = cap->data.scsi_host.wwnn;
> +            *wwpn = cap->data.scsi_host.wwnn;
> +            break;
> +        }
> +
> +        cap = cap->next;
> +    }
> +
> +    if (cap == NULL) {
> +        /* XXX This error code is wrong--it results in errors of the form:
> +           "error: invalid node device pointer in Device foo is not a fibre channel HBA"
> +        */
> +        virNodeDeviceReportError(conn, VIR_ERR_INVALID_NODE_DEVICE,
> +                                 _("Device %s is not a fibre channel HBA"),
> +                                 def->name);
> +        ret = -1;
> +    }
> +
> +    return ret;
> +}

What we really want to indicate is that we only support device creation
for SCSI vports. For everything else we need to return VIR_ERR_NO_SUPPORT
with a message along the lines of 'This type of device cannot be created'


The rest of the patch looks pretty reasonable to me. Only thing missing
was an update to docs/schemas/nodedev.rng to list the new attributes
that are allowed

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 :|