[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



Daniel Veillard wrote:
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.

Thanks--that does make the code flow better.

+                        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

Already done.

+    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

DanPB made the same comment; fixed.

[...]
+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:

The unlock can be moved before the out: label and the conditional removed, so I think that's the cleanest way to do it.

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

That's correct.

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

better to fully parenthesize

Huh--funny, I usually do.  Thanks for pointing that out.  Fixed.

+        /* 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 !

This point is interesting. It turns out that I didn't need to release the lock when calling wait for devices; I'm not sure where I got that idea. In any case, do I need to be holding the driver lock throughout the entire node device create operation? I think no, as the driver pointer is not going to be invalidated during the node device create operation, and I don't really care if the structure it points to changes, as I'm taking the lock again when I look to see if the device was created.

If the lock *does* need to be held through the entire operation, then the 5 seconds is nothing compared to the 180 seconds that udev might take to settle out. In my devel system, it routinely takes > 30 seconds because I have so many devices and am creating so many new devices with each create call. If that's the case, then I think we need to provide an async option to the call that returns without the dev pointer and expects the caller to poll for it.

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

Agreed, that should be a separate patch.

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

Fixed, thank you for catching that.


+    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

I'll check the return status; not sure what I was thinking there.

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

I need to add the documentation.

Dave


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