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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list