Re: [Libvir] PATCH: 13/16: iSCSI backend
A couple of comments about this, inline.
<snip>
+static int virStorageBackendISCSIConnection(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
+ const char *portal,
+ const char *action)
+{
+ const char *cmdargv[] = {
+ ISCSIADM, "--mode", "node", "--portal", portal,
+ "--targetname", pool->def->source.devices[0].path, action, NULL
+ };
+
+ if (virRun(conn, (char **)cmdargv, NULL) < 0)
+ return -1;
+
+ return 0;
+}
Dan and I discussed this on IRC already, but the above code won't work
as-is. The way iscsiadm works is that you first scan the target with an
"iscsiadm --mode discovery --type sendtargets --portal <ip>". This
command actually creates a directory structure which later iscsiadm
commands (including --login) use. So you basically have to do the
sendtargets, even if you already know the name of the target you want to
connect to. It's just a bugfix, however, and shouldn't prevent this
from going in.
<snip>
+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.
+
+
+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.
<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.
Chris Lalancette