On Tue, Feb 12, 2008 at 08:37:02AM -0500, Chris Lalancette wrote: > > +static char *virStorageBackendISCSIPortal(virConnectPtr conn, > > + virStoragePoolObjPtr pool) > > +{ > > + char ipaddr[NI_MAXHOST]; > > + char *portal; > > + > > + if (virStorageBackendISCSITargetIP(conn, > > + pool->def->source.host.name, > > + ipaddr, sizeof(ipaddr)) < 0) > > + return NULL; > > + > > + portal = malloc(strlen(ipaddr) + 1 + 4 + 2 + 1); > > + if (portal == NULL) { > > + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "portal"); > > + return NULL; > > + } > > + > > + strcpy(portal, ipaddr); > > + strcat(portal, ":3260,1"); > > + > > + return portal; > > +} > > We probably shouldn't hardcode the 3260 (port number). I have no idea > how common it is to run iSCSI servers on other ports, but I have to > imagine it is possible, so we should probably allow it. Again, though, > this seems like it would just be an extension in the XML that we would > use (adding a port="" attribute), so not something that would block this > going in. The internal storage_conf.h file does actually track the port number, but I need to add the XML parser code to extract it & then hook it up here. It should not be hard - just on my TODO list. > > +static int virStorageBackendISCSIStartPool(virConnectPtr conn, > > + virStoragePoolObjPtr pool) > > +{ > > + char *portal = NULL; > > + > > + if (pool->def->source.host.name == NULL) { > > + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "missing source host"); > > + return -1; > > + } > > + > > + if (pool->def->source.ndevice != 1 || > > + pool->def->source.devices[0].path == NULL) { > > + virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "missing source device"); > > + return -1; > > + } > > + > > + if ((portal = virStorageBackendISCSIPortal(conn, pool)) == NULL) > > + return -1; > > + if (virStorageBackendISCSILogin(conn, pool, portal) < 0) { > > + free(portal); > > + return -1; > > + } > > + free(portal); > > + return 0; > > +} > > One general comment I have here; it doesn't seem it is possible to > specify an iSCSI server without a target name, which is a little > unfortunate. What would be really nice would be able to give an IP > address + port number to this driver, and then have it automatically > scan all targets (via the sendtargets discussed above) and either return > that list to the user, or store it internally for later use. I have no > idea how this would fit in with the current API, however. That's also a feature I intend to add. The current system lets you define storage pools from a config you have prepared. There is also a method virConnectDiscoverStoragePools whose purpose is for autodiscovery. For iSCSI it would query for all exported targets, and give you back a list of XML docs representing a suitable defautl pool config for each target. For NFS it would query for all exported shares. For LVM it would lookup all defined volume groups. For disks, it would return configs for all local disks, etc etc. > <snip> > > +static int virStorageBackendISCSIStopPool(virConnectPtr conn, > > + virStoragePoolObjPtr pool) > > +{ > > + char *portal; > > + > > + if ((portal = virStorageBackendISCSIPortal(conn, pool)) == NULL) > > + return -1; > > + > > + if (virStorageBackendISCSILogout(conn, pool, portal) < 0) { > > + free(portal); > > + return -1; > > + } > > + free(portal); > > + > > + return 0; > > +} > > One note for testers; current 2.6.23 and 2.6.24 kernels have a bug where > iscsiadm will hang on --logout. This should be fixed in 2.6.25, but it > will cause problems until then when stopping a pool. It'll probably fail on 2.6.21 and 2.6.22 too. I've been testing iSCSI on RHEL-5 with 2.6.18 primarily. The fixes will be in 2.6.25 Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=| -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list