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