[libvirt] [PATCH 1/2] Backend of node device API NPIV support
Dave Allan
dallan at redhat.com
Thu May 14 22:01:03 UTC 2009
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
More information about the libvir-list
mailing list