Re: [Libvir] PATCH: 13/16: iSCSI backend
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 -=|