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(a)veillard.com | Rpmfind RPM search engine
http://rpmfind.net/
http://veillard.com/ | virtualization library
http://libvirt.org/