[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Fri, May 08, 2009 at 05:41:08PM -0400, David Allan wrote:
> +/* Caller must hold the driver lock. */
> +static virNodeDevicePtr
> +nodeDeviceLookupByWWN(virConnectPtr conn,
> +                      const char *wwnn,
> +                      const char *wwpn)
> +{
> +    unsigned int i, found = 0;
> +    virDeviceMonitorStatePtr driver = conn->devMonPrivateData;
> +    virNodeDeviceObjListPtr devs = &driver->devs;
> +    virNodeDevCapsDefPtr cap = NULL;
> +    virNodeDeviceObjPtr obj = NULL;
> +    virNodeDevicePtr dev = NULL;
> +
> +    for (i = 0; i < devs->count; i++) {
> +
> +        obj = devs->objs[i];
> +        virNodeDeviceObjLock(obj);
> +        cap = obj->def->caps;
> +
> +        while (cap) {
> +
> +            if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST) {
> +                if (cap->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
> +                    if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
> +                        STREQ(cap->data.scsi_host.wwpn, wwpn)) {

  I would fetch dev and call virNodeDeviceObjUnlock(obj) here before
  getting to out: i.e. keep the crucial actions closer to the spot and
  stay generic in the exit section.

> +                        found = 1;
> +                        goto out;
> +                    }
> +                }
> +            }
> +            cap = cap->next;
> +        }
> +
> +        virNodeDeviceObjUnlock(obj);
> +    }
> +
> +out:
> +    if (found) {
> +        dev = virGetNodeDevice(conn, obj->def->name);
> +        virNodeDeviceObjUnlock(obj);
> +    }
> +
> +    return dev;
> +}
> +
> +
>  static char *nodeDeviceDumpXML(virNodeDevicePtr dev,
>                                 unsigned int flags ATTRIBUTE_UNUSED)
>  {
> @@ -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;

  I would initilize it to NULL

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

  and unify the 3 exit labels with
    if (fd >= 0)
        close(fd)
    VIR_FREE(operation_path);
    VIR_DEBUG(...)

if you initilize the variable on enter like fd you can have a single
exit point, simpler code, less prone to breakage if the code is modified
later

[...]
> +static int
> +get_parent_host(virConnectPtr conn,
> +                virDeviceMonitorStatePtr driver,
> +                virNodeDeviceDefPtr def,
> +                int *parent_host)
> +{
> +    virNodeDeviceObjPtr parent = NULL;
> +    virNodeDevCapsDefPtr cap = NULL;
> +    int ret = 0;
> +
> +    parent = virNodeDeviceFindByName(&driver->devs, def->parent);
> +    if (parent == NULL) {
> +        virNodeDeviceReportError(conn, VIR_ERR_INVALID_NODE_DEVICE,
> +                                 _("Could not find parent device for '%s'"),
> +                                 def->name);
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    cap = parent->def->caps;
> +    while (cap != NULL) {
> +        if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST) {
> +            *parent_host = cap->data.scsi_host.host;
> +            break;
> +        }
> +
> +        cap = cap->next;
> +    }
> +
> +    if (cap == NULL) {
> +        virNodeDeviceReportError(conn, VIR_ERR_INVALID_NODE_DEVICE,
> +                                 _("Device %s is not a SCSI host"),
> +                                 parent->def->name);
> +        ret = -1;
> +    }
> +
> +out:
> +    if (parent != NULL) {
> +        virNodeDeviceObjUnlock(parent);
> +    }

  again I would move it before the break and replace the break to a goto
  out:

I find a bit weird to have only unlocking in this function, but I assume
it's just because we get a locked parent as a result of FindByName

> +
> +    return ret;
> +}
> +
[...]
> +    while (dev == NULL && now - start < LINUX_NEW_DEVICE_WAIT_TIME) {

              better to fully parenthesize 

> +        /* We can't hold the driver lock while we wait because the
> +           wait for devices call takes it.  It's safe to drop the lock
> +           because we're done with the driver structure at this point
> +           anyway.  We take it again when we look to see what, if
> +           anything, was created. */
> +        nodeDeviceUnlock(driver);
> +        virNodeDeviceWaitForDevices(conn);
> +        nodeDeviceLock(driver);
> +
> +        dev = nodeDeviceLookupByWWN(conn, wwnn, wwpn);
> +
> +        if (dev == NULL) {
> +            sleep(5);

  Hum, we do sleep with the lock here, and for a long time !

> +            now = time(NULL);
> +            if (now == (time_t)-1) {
> +                virNodeDeviceReportError(dev->conn, VIR_ERR_INTERNAL_ERROR,
> +                                         "%s", _("Could not get current time"));
> +                break;
> +            }
> +        }
> +    }
> +
> +    return dev;
> +}
[...]
> --- a/src/node_device_conf.c
> +++ b/src/node_device_conf.c
> @@ -53,9 +53,34 @@ VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST,
>                "80203",
>                "80211")
>  
> +VIR_ENUM_IMPL(virNodeDevHBACap, VIR_NODE_DEV_CAP_HBA_LAST,
> +              "fc_host",
> +              "vport_ops")
>  
>  #define virNodeDeviceLog(msg...) fprintf(stderr, msg)

  errr, can we fix that while modifying this module, we really ought
to use the normal logging internal APIs
  Well there are other places:

network_driver.c:#define networkLog(level, msg...) fprintf(stderr, msg)
node_device_conf.c:#define virNodeDeviceLog(msg...) fprintf(stderr, msg)
storage_conf.c:#define virStorageLog(msg...) fprintf(stderr, msg)
storage_driver.c:#define storageLog(msg...) fprintf(stderr, msg)

  I will rather do a separate PATCH to plug those...

> +    if ((fd = open(wwnn_path, O_RDONLY)) < 0) {
> +        goto out;
> +    }
> +
> +    memset(buf, 0, sizeof(buf));
> +    if (saferead(fd, buf, sizeof(buf)) < 0) {
> +        goto out;

  here fd is open but out: doesn't close it.

> +    }
> +
> +    close(fd);

  just set back fd to -1 here and in other places where you close it
and in out: add
    if (fd >= 0)
        close(fd);


> +    if ((fd = open(wwpn_path, O_RDONLY)) < 0) {
> +        goto out;
> +    }
> +
> +    memset(buf, 0, sizeof(buf));
> +    if (saferead(fd, buf, sizeof(buf)) < 0) {
> +        goto out;

  same here. Alternatively just close(fd) before goto out; in those 2
  cases.


>  static int gather_scsi_host_cap(LibHalContext *ctx, const char *udi,
>                                  union _virNodeDevCapData *d)
>  {
>      (void)get_int_prop(ctx, udi, "scsi_host.host", (int *)&d->scsi_host.host);
> +    (void)check_fc_host(d);
> +    (void)check_vport_capable(d);
>      return 0;
>  }

  Hum, I find hard to justify those void casts

One thing I find missing is that we extend the capabilities but I don't
see any associated documentation, or is that already covered ?

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]