The XML docs describe a 'port' attribute for the storage source <host> element, but the parser never handled it. * docs/schemas/storagepool.rng: Define port attribute * src/conf/storage_conf.c: Add missing parsing/formatting of host port number * src/conf/storage_conf.h: Remove bogus/unused 'protocol' field --- docs/schemas/storagepool.rng | 5 +++++ src/conf/storage_conf.c | 21 +++++++++++++++++++-- src/conf/storage_conf.h | 1 - 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 54eb802..8f067f3 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -186,6 +186,11 @@ <attribute name='name'> <text/> </attribute> + <optional> + <attribute name='port'> + <text/> + </attribute> + </optional> <empty/> </element> </define> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0678905..45285de 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -396,6 +396,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, char *authType = NULL; int nsource, i; virStoragePoolOptionsPtr options; + char *port; relnode = ctxt->node; ctxt->node = node; @@ -423,6 +424,17 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, } source->host.name = virXPathString("string(./host/@name)", ctxt); + port = virXPathString("string(./host/@port)", ctxt); + if (port) { + if (virStrToLong_i(port, NULL, 10, &source->host.port) < 0) { + virStorageReportError(VIR_ERR_XML_ERROR, + _("Invalid port number: %s"), + port); + goto cleanup; + } + } + + source->initiator.iqn = virXPathString("string(./initiator/iqn/@name)", ctxt); nsource = virXPathNodeSet("./device", ctxt, &nodeset); @@ -475,6 +487,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, cleanup: ctxt->node = relnode; + VIR_FREE(port); VIR_FREE(authType); VIR_FREE(nodeset); return ret; @@ -790,8 +803,12 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAddLit(buf," <source>\n"); if ((options->flags & VIR_STORAGE_POOL_SOURCE_HOST) && - src->host.name) - virBufferVSprintf(buf," <host name='%s'/>\n", src->host.name); + src->host.name) { + virBufferVSprintf(buf, " <host name='%s'", src->host.name); + if (src->host.port) + virBufferVSprintf(buf, " port='%d'", src->host.port); + virBufferAddLit(buf, "/>\n"); + } if ((options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) && src->ndevice) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index fef0a46..9e1f534 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -155,7 +155,6 @@ typedef virStoragePoolSourceHost *virStoragePoolSourceHostPtr; struct _virStoragePoolSourceHost { char *name; int port; - int protocol; }; -- 1.7.2.3

On Fri, Nov 12, 2010 at 10:21:16AM -0700, Eric Blake wrote: As per the commit message, this is already documented, but not implemented ! I guess it should reference a port number The range check gets done eventually, at least by the kernel, so I'm not too bothered about that. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

The following series of patches are adding significant extra functionality to the iSCSI driver. THe current internal helper methods are not sufficiently flexible to cope with these changes. This patch refactors the code to avoid needing to have a virStoragePoolObjPtr instance as a parameter, instead passing individual target, portal and initiatoriqn parameters. It also removes hardcoding of port 3260 in the portal address, instead using the XML value if any. * src/storage/storage_backend_iscsi.c: Refactor internal helper methods --- src/storage/storage_backend_iscsi.c | 251 +++++++++++++++++------------------ 1 files changed, 125 insertions(+), 126 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 06a89ec..51f71af 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -89,6 +89,27 @@ virStorageBackendISCSITargetIP(const char *hostname, return 0; } +static char * +virStorageBackendISCSIPortal(virStoragePoolSourcePtr source) +{ + char ipaddr[NI_MAXHOST]; + char *portal; + + if (virStorageBackendISCSITargetIP(source->host.name, + ipaddr, sizeof(ipaddr)) < 0) + return NULL; + + if (virAsprintf(&portal, "%s:%d,1", ipaddr, + source->host.port ? + source->host.port : 3260) < 0) { + virReportOOMError(); + return NULL; + } + + return portal; +} + + static int virStorageBackendISCSIExtractSession(virStoragePoolObjPtr pool, char **const groups, @@ -156,7 +177,7 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, #define LINE_SIZE 4096 static int -virStorageBackendIQNFound(virStoragePoolObjPtr pool, +virStorageBackendIQNFound(const char *initiatoriqn, char **ifacename) { int ret = IQN_MISSING, fd = -1; @@ -182,7 +203,7 @@ virStorageBackendIQNFound(virStoragePoolObjPtr pool, if (virExec(prog, NULL, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to run '%s' when looking for existing interface with IQN '%s'"), - prog[0], pool->def->source.initiator.iqn); + prog[0], initiatoriqn); ret = IQN_ERROR; goto out; @@ -215,7 +236,7 @@ virStorageBackendIQNFound(virStoragePoolObjPtr pool, } iqn++; - if (STREQ(iqn, pool->def->source.initiator.iqn)) { + if (STREQ(iqn, initiatoriqn)) { token = strtok_r(line, " ", &saveptr); *ifacename = strdup(token); if (*ifacename == NULL) { @@ -246,7 +267,7 @@ out: static int -virStorageBackendCreateIfaceIQN(virStoragePoolObjPtr pool, +virStorageBackendCreateIfaceIQN(const char *initiatoriqn, char **ifacename) { int ret = -1, exitstatus = -1; @@ -267,7 +288,7 @@ virStorageBackendCreateIfaceIQN(virStoragePoolObjPtr pool, }; VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'", - &temp_ifacename[0], pool->def->source.initiator.iqn); + &temp_ifacename[0], initiatoriqn); /* Note that we ignore the exitstatus. Older versions of iscsiadm * tools returned an exit status of > 0, even if they succeeded. @@ -283,7 +304,7 @@ virStorageBackendCreateIfaceIQN(virStoragePoolObjPtr pool, const char *const cmdargv2[] = { ISCSIADM, "--mode", "iface", "--interface", &temp_ifacename[0], "--op", "update", "--name", "iface.initiatorname", "--value", - pool->def->source.initiator.iqn, NULL + initiatoriqn, NULL }; /* Note that we ignore the exitstatus. Older versions of iscsiadm tools @@ -292,19 +313,19 @@ virStorageBackendCreateIfaceIQN(virStoragePoolObjPtr pool, if (virRun(cmdargv2, &exitstatus) < 0) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), - cmdargv2[0], pool->def->source.initiator.iqn); + cmdargv2[0], initiatoriqn); goto out; } /* Check again to make sure the interface was created. */ - if (virStorageBackendIQNFound(pool, ifacename) != IQN_FOUND) { + if (virStorageBackendIQNFound(initiatoriqn, ifacename) != IQN_FOUND) { VIR_DEBUG("Failed to find interface '%s' with IQN '%s' " "after attempting to create it", - &temp_ifacename[0], pool->def->source.initiator.iqn); + &temp_ifacename[0], initiatoriqn); goto out; } else { VIR_DEBUG("Interface '%s' with IQN '%s' was created successfully", - *ifacename, pool->def->source.initiator.iqn); + *ifacename, initiatoriqn); } ret = 0; @@ -316,82 +337,72 @@ out: } + static int -virStorageBackendISCSIConnectionIQN(virStoragePoolObjPtr pool, - const char *portal, - const char *action) +virStorageBackendISCSIConnection(const char *portal, + const char *initiatoriqn, + const char *target, + const char **extraargv) { int ret = -1; + const char *const baseargv[] = { + ISCSIADM, + "--mode", "node", + "--portal", portal, + "--targetname", target, + NULL + }; + int i; + int nargs = 0; char *ifacename = NULL; + const char **cmdargv; - switch (virStorageBackendIQNFound(pool, &ifacename)) { - case IQN_FOUND: - VIR_DEBUG("ifacename: '%s'", ifacename); - break; - case IQN_MISSING: - if (virStorageBackendCreateIfaceIQN(pool, &ifacename) != 0) { - goto out; - } - break; - case IQN_ERROR: - default: - goto out; - } + for (i = 0 ; baseargv[i] != NULL ; i++) + nargs++; + for (i = 0 ; extraargv[i] != NULL ; i++) + nargs++; + if (initiatoriqn) + nargs += 2; - const char *const sendtargets[] = { - ISCSIADM, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, NULL - }; - if (virRun(sendtargets, NULL) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run %s to get target list"), - sendtargets[0]); - goto out; + if (VIR_ALLOC_N(cmdargv, nargs+1) < 0) { + virReportOOMError(); + return -1; } - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, "--interface", - ifacename, action, NULL - }; + nargs = 0; + for (i = 0 ; baseargv[i] != NULL ; i++) + cmdargv[nargs++] = baseargv[i]; + for (i = 0 ; extraargv[i] != NULL ; i++) + cmdargv[nargs++] = extraargv[i]; - if (virRun(cmdargv, NULL) < 0) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run command '%s' with action '%s'"), - cmdargv[0], action); - goto out; + if (initiatoriqn) { + switch (virStorageBackendIQNFound(initiatoriqn, &ifacename)) { + case IQN_FOUND: + VIR_DEBUG("ifacename: '%s'", ifacename); + break; + case IQN_MISSING: + if (virStorageBackendCreateIfaceIQN(initiatoriqn, &ifacename) != 0) { + goto cleanup; + } + break; + case IQN_ERROR: + default: + goto cleanup; + } + + cmdargv[nargs++] = "--interface"; + cmdargv[nargs++] = ifacename; } + cmdargv[nargs++] = NULL; + + if (virRun(cmdargv, NULL) < 0) + goto cleanup; ret = 0; -out: +cleanup: + VIR_FREE(cmdargv); VIR_FREE(ifacename); - return ret; -} - - -static int -virStorageBackendISCSIConnection(virStoragePoolObjPtr pool, - const char *portal, - const char *action) -{ - int ret = 0; - - if (pool->def->source.initiator.iqn != NULL) { - - ret = virStorageBackendISCSIConnectionIQN(pool, portal, action); - - } else { - - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(cmdargv, NULL) < 0) { - ret = -1; - } - - } return ret; } @@ -441,46 +452,18 @@ virStorageBackendISCSIRescanLUNs(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, static int -virStorageBackendISCSILogin(virStoragePoolObjPtr pool, - const char *portal) +virStorageBackendISCSIScanTargets(const char *portal) { - const char *const cmdsendtarget[] = { - ISCSIADM, "--mode", "discovery", "--type", "sendtargets", - "--portal", portal, NULL + const char *const sendtargets[] = { + ISCSIADM, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, NULL }; - - if (virRun(cmdsendtarget, NULL) < 0) + if (virRun(sendtargets, NULL) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to run %s to get target list"), + sendtargets[0]); return -1; - - return virStorageBackendISCSIConnection(pool, portal, "--login"); -} - -static int -virStorageBackendISCSILogout(virStoragePoolObjPtr pool, - const char *portal) -{ - return virStorageBackendISCSIConnection(pool, portal, "--logout"); -} - -static char * -virStorageBackendISCSIPortal(virStoragePoolObjPtr pool) -{ - char ipaddr[NI_MAXHOST]; - char *portal; - - if (virStorageBackendISCSITargetIP(pool->def->source.host.name, - ipaddr, sizeof(ipaddr)) < 0) - return NULL; - - if (VIR_ALLOC_N(portal, strlen(ipaddr) + 1 + 4 + 2 + 1) < 0) { - virReportOOMError(); - return NULL; } - - strcpy(portal, ipaddr); - strcat(portal, ":3260,1"); - - return portal; + return 0; } @@ -489,7 +472,9 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { char *portal = NULL; - char *session; + char *session = NULL; + int ret = -1; + const char *loginargv[] = { "--login", NULL }; if (pool->def->source.host.name == NULL) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, @@ -505,17 +490,26 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, } if ((session = virStorageBackendISCSISession(pool, 1)) == NULL) { - if ((portal = virStorageBackendISCSIPortal(pool)) == NULL) - return -1; - if (virStorageBackendISCSILogin(pool, portal) < 0) { - VIR_FREE(portal); - return -1; - } - VIR_FREE(portal); - } else { - VIR_FREE(session); + if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) + goto cleanup; + /* + * iscsiadm doesn't let you login to a target, unless you've + * first issued a 'sendtargets' command to the portal :-( + */ + if (virStorageBackendISCSIScanTargets(portal) < 0) + goto cleanup; + + if (virStorageBackendISCSIConnection(portal, + pool->def->source.initiator.iqn, + pool->def->source.devices[0].path, + loginargv) < 0) + goto cleanup; } - return 0; + ret = 0; + +cleanup: + VIR_FREE(session); + return ret; } static int @@ -546,18 +540,23 @@ static int virStorageBackendISCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { + const char *logoutargv[] = { "--logout", NULL }; char *portal; + int ret = -1; - if ((portal = virStorageBackendISCSIPortal(pool)) == NULL) + if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) return -1; - if (virStorageBackendISCSILogout(pool, portal) < 0) { - VIR_FREE(portal); - return -1; - } - VIR_FREE(portal); + if (virStorageBackendISCSIConnection(portal, + pool->def->source.initiator.iqn, + pool->def->source.devices[0].path, + logoutargv) < 0) + goto cleanup; + ret = 0; - return 0; +cleanup: + VIR_FREE(portal); + return ret; } virStorageBackend virStorageBackendISCSI = { -- 1.7.2.3

The Linux iSCSI initiator toolchain has the dubious feature that if you ever run the 'sendtargets' command to merely query what targets are available from a server, the results will be recorded in /var/lib/iscsi. Any time the '/etc/init.d/iscsi' script runs in the future, it will then automatically login to all those targets. /etc/init.d/iscsi is automatically run whenever a NIC comes online. So from the moment you ask a server what targets are available, your client will forever more automatically try to login to all targets without ever asking if you actually want it todo this. To stop this stupid behaviour, we need to run iscsiadm --portal $PORTAL --target $TARGET --op update --name node.startup --value manual For every target on the server. * src/storage/storage_backend_iscsi.c: Disable automatic login for targets found as a result of a 'sendtargets' command --- src/storage/storage_backend_iscsi.c | 120 +++++++++++++++++++++++++++++++++-- 1 files changed, 113 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 51f71af..2a36527 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -450,19 +450,123 @@ virStorageBackendISCSIRescanLUNs(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, return 0; } +struct virStorageBackendISCSITargetList { + size_t ntargets; + char **targets; +}; + +static int +virStorageBackendISCSIGetTargets(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + char **const groups, + void *data) +{ + struct virStorageBackendISCSITargetList *list = data; + char *target; + + if (!(target = strdup(groups[1]))) { + virReportOOMError(); + return -1; + } + + if (VIR_REALLOC_N(list->targets, list->ntargets + 1) < 0) { + VIR_FREE(target); + virReportOOMError(); + return -1; + } + + list->targets[list->ntargets] = target; + list->ntargets++; + + return 0; +} + +static int +virStorageBackendISCSITargetAutologin(const char *portal, + const char *initiatoriqn, + const char *target, + bool enable) +{ + const char *extraargv[] = { "--op", "update", + "--name", "node.startup", + "--value", enable ? "automatic" : "manual", + NULL }; + + return virStorageBackendISCSIConnection(portal, initiatoriqn, target, extraargv); +} + static int -virStorageBackendISCSIScanTargets(const char *portal) +virStorageBackendISCSIScanTargets(const char *portal, + const char *initiatoriqn, + size_t *ntargetsret, + char ***targetsret) { - const char *const sendtargets[] = { - ISCSIADM, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, NULL + /** + * + * The output of sendtargets is very simple, just two columns, + * portal then target name + * + * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo0.bf6d84 + * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo1.bf6d84 + * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo2.bf6d84 + * 192.168.122.185:3260,1 iqn.2004-04.com:fedora14:iscsi.demo3.bf6d84 + */ + const char *regexes[] = { + "^\\s*(\\S+)\\s+(\\S+)\\s*$" + }; + int vars[] = { 2 }; + const char *const cmdsendtarget[] = { + ISCSIADM, "--mode", "discovery", "--type", "sendtargets", + "--portal", portal, NULL }; - if (virRun(sendtargets, NULL) < 0) { + struct virStorageBackendISCSITargetList list; + int i; + int exitstatus; + + memset(&list, 0, sizeof(list)); + + if (virStorageBackendRunProgRegex(NULL, /* No pool for callback */ + cmdsendtarget, + 1, + regexes, + vars, + virStorageBackendISCSIGetTargets, + &list, + &exitstatus) < 0) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run %s to get target list"), - sendtargets[0]); + "%s", _("lvs command failed")); + return -1; + } + + if (exitstatus != 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("iscsiadm sendtargets command failed with exitstatus %d"), + exitstatus); return -1; } + + for (i = 0 ; i < list.ntargets ; i++) { + /* We have to ignore failure, because we can't undo + * the results of 'sendtargets', unless we go scrubbing + * around in the dirt in /var/lib/iscsi. + */ + if (virStorageBackendISCSITargetAutologin(portal, + initiatoriqn, + list.targets[i], false) < 0) + VIR_WARN("Unable to disable auto-login on iSCSI target %s: %s", + portal, list.targets[i]); + } + + if (ntargetsret && targetsret) { + *ntargetsret = list.ntargets; + *targetsret = list.targets; + } else { + for (i = 0 ; i < list.ntargets ; i++) { + VIR_FREE(list.targets[i]); + } + VIR_FREE(list.targets); + } + return 0; } @@ -496,7 +600,9 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, * iscsiadm doesn't let you login to a target, unless you've * first issued a 'sendtargets' command to the portal :-( */ - if (virStorageBackendISCSIScanTargets(portal) < 0) + if (virStorageBackendISCSIScanTargets(portal, + pool->def->source.initiator.iqn, + NULL, NULL) < 0) goto cleanup; if (virStorageBackendISCSIConnection(portal, -- 1.7.2.3

On 11/12/2010 09:22 AM, Daniel P. Berrange wrote: Yep, answering my own question on patch 5/11, you stuck something in the middle. ACK. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 11/12/2010 09:22 AM, Daniel P. Berrange wrote: Slight change which may break existing scripts that used an undocumented method, but makes sense given that the documentation calls out both arguments, and in light of IPv6 hostnames. However, given that we are no longer computing hostlen as anything other than strlen(host), I would recommend that you respin this patch to completely get rid of hostlen, and use %s instead of %.*s. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

When libvirt starts up all storage pools default to the inactive state, even if the underlying storage is already active on the host. This introduces a new API into the internal storage backend drivers that checks whether a storage pool is already active. If the pool is active at libvirtd startup, the volume list will be immediately populated. * src/storage/storage_backend.h: New internal API for checking storage pool state * src/storage/storage_driver.c: Check whether a pool is active upon driver startup * src/storage/storage_backend_fs.c, src/storage/storage_backend_iscsi.c, src/storage/storage_backend_logical.c, src/storage/storage_backend_mpath.c, src/storage/storage_backend_scsi.c: Add checks for pool state --- src/storage/storage_backend.h | 3 ++ src/storage/storage_backend_fs.c | 29 +++++++++++++++++++++++++++- src/storage/storage_backend_iscsi.c | 34 +++++++++++++++++++++++++++++++++ src/storage/storage_backend_logical.c | 24 +++++++++++++++++++++++ src/storage/storage_backend_mpath.c | 20 +++++++++++++++++++ src/storage/storage_backend_scsi.c | 25 ++++++++++++++++++++++++ src/storage/storage_driver.c | 31 ++++++++++++++++++++++------- 7 files changed, 157 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 1165a45..6f395c7 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -25,10 +25,12 @@ # define __VIR_STORAGE_BACKEND_H__ # include <stdint.h> +# include <stdbool.h> # include "internal.h" # include "storage_conf.h" typedef char * (*virStorageBackendFindPoolSources)(virConnectPtr conn, const char *srcSpec, unsigned int flags); +typedef int (*virStorageBackendCheckPool)(virConnectPtr conn, virStoragePoolObjPtr pool, bool *active); typedef int (*virStorageBackendStartPool)(virConnectPtr conn, virStoragePoolObjPtr pool); typedef int (*virStorageBackendBuildPool)(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags); typedef int (*virStorageBackendRefreshPool)(virConnectPtr conn, virStoragePoolObjPtr pool); @@ -65,6 +67,7 @@ struct _virStorageBackend { int type; virStorageBackendFindPoolSources findPoolSources; + virStorageBackendCheckPool checkPool; virStorageBackendStartPool startPool; virStorageBackendBuildPool buildPool; virStorageBackendRefreshPool refreshPool; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c2bc6c4..8cdf7c0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -479,6 +479,31 @@ virStorageBackendFileSystemUnmount(virStoragePoolObjPtr pool) { #endif /* WITH_STORAGE_FS */ +static int +virStorageBackendFileSystemCheck(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + bool *isActive) +{ + *isActive = false; + if (pool->def->type == VIR_STORAGE_POOL_DIR) { + struct stat sb; + if (stat(pool->def->target.path, &sb) == 0) + *isActive = true; +#if WITH_STORAGE_FS + } else { + int ret; + if ((ret = virStorageBackendFileSystemIsMounted(pool)) != 0) { + if (ret < 0) + return -1; + *isActive = true; + } +#endif /* WITH_STORAGE_FS */ + } + + return 0; +} + +#if WITH_STORAGE_FS /** * @conn connection to report errors against * @pool storage pool to start @@ -489,7 +514,6 @@ virStorageBackendFileSystemUnmount(virStoragePoolObjPtr pool) { * * Returns 0 on success, -1 on error */ -#if WITH_STORAGE_FS static int virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) @@ -937,6 +961,7 @@ virStorageBackend virStorageBackendDirectory = { .type = VIR_STORAGE_POOL_DIR, .buildPool = virStorageBackendFileSystemBuild, + .checkPool = virStorageBackendFileSystemCheck, .refreshPool = virStorageBackendFileSystemRefresh, .deletePool = virStorageBackendFileSystemDelete, .buildVol = virStorageBackendFileSystemVolBuild, @@ -951,6 +976,7 @@ virStorageBackend virStorageBackendFileSystem = { .type = VIR_STORAGE_POOL_FS, .buildPool = virStorageBackendFileSystemBuild, + .checkPool = virStorageBackendFileSystemCheck, .startPool = virStorageBackendFileSystemStart, .refreshPool = virStorageBackendFileSystemRefresh, .stopPool = virStorageBackendFileSystemStop, @@ -965,6 +991,7 @@ virStorageBackend virStorageBackendNetFileSystem = { .type = VIR_STORAGE_POOL_NETFS, .buildPool = virStorageBackendFileSystemBuild, + .checkPool = virStorageBackendFileSystemCheck, .startPool = virStorageBackendFileSystemStart, .findPoolSources = virStorageBackendFileSystemNetFindPoolSources, .refreshPool = virStorageBackendFileSystemRefresh, diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index a67a428..2dcf714 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -639,6 +639,39 @@ cleanup: } static int +virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + bool *isActive) +{ + char *session = NULL; + int ret = -1; + + *isActive = false; + + if (pool->def->source.host.name == NULL) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing source host")); + return -1; + } + + if (pool->def->source.ndevice != 1 || + pool->def->source.devices[0].path == NULL) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing source device")); + return -1; + } + + if ((session = virStorageBackendISCSISession(pool, 1)) != NULL) { + *isActive = true; + VIR_FREE(session); + } + ret = 0; + + return ret; +} + + +static int virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { @@ -735,6 +768,7 @@ cleanup: virStorageBackend virStorageBackendISCSI = { .type = VIR_STORAGE_POOL_ISCSI, + .checkPool = virStorageBackendISCSICheckPool, .startPool = virStorageBackendISCSIStartPool, .refreshPool = virStorageBackendISCSIRefreshPool, .stopPool = virStorageBackendISCSIStopPool, diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index e6c6938..c5bfda1 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -24,6 +24,7 @@ #include <config.h> #include <sys/wait.h> +#include <sys/types.h> #include <sys/stat.h> #include <stdio.h> #include <regex.h> @@ -361,6 +362,28 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, static int +virStorageBackendLogicalCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + bool *isActive) +{ + char *path; + + *isActive = false; + if (virAsprintf(&path, "/dev/%s", pool->def->source.name) < 0) { + virReportOOMError(); + return -1; + } + + struct stat sb; + if (stat(path, &sb) == 0) + *isActive = true; + + VIR_FREE(path); + + return 0; +} + +static int virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { @@ -684,6 +707,7 @@ virStorageBackend virStorageBackendLogical = { .type = VIR_STORAGE_POOL_LOGICAL, .findPoolSources = virStorageBackendLogicalFindPoolSources, + .checkPool = virStorageBackendLogicalCheckPool, .startPool = virStorageBackendLogicalStartPool, .buildPool = virStorageBackendLogicalBuildPool, .refreshPool = virStorageBackendLogicalRefreshPool, diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 79ad4b8..d55a1c4 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -27,6 +27,8 @@ #include <stdio.h> #include <dirent.h> #include <fcntl.h> +#include <sys/stat.h> +#include <sys/types.h> #include <libdevmapper.h> @@ -291,6 +293,23 @@ out: return retval; } +static int +virStorageBackendMpathCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + bool *isActive) +{ + const char *path = "/dev/mpath"; + + *isActive = false; + + struct stat sb; + if (stat(path, &sb) == 0) + *isActive = true; + + return 0; +} + + static int virStorageBackendMpathRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -313,5 +332,6 @@ virStorageBackendMpathRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStorageBackend virStorageBackendMpath = { .type = VIR_STORAGE_POOL_MPATH, + .checkPool = virStorageBackendMpathCheckPool, .refreshPool = virStorageBackendMpathRefreshPool, }; diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 28d6ac6..0d6b1ac 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -27,6 +27,9 @@ #include <stdio.h> #include <dirent.h> #include <fcntl.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <sys/wait.h> #include "virterror_internal.h" #include "storage_backend_scsi.h" @@ -587,6 +590,27 @@ out: return retval; } +static int +virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + bool *isActive) +{ + char *path; + + *isActive = false; + if (virAsprintf(&path, "/sys/class/scsi_host/%s", pool->def->source.adapter) < 0) { + virReportOOMError(); + return -1; + } + + struct stat sb; + if (stat(path, &sb) == 0) + *isActive = true; + + VIR_FREE(path); + + return 0; +} static int virStorageBackendSCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -621,5 +645,6 @@ out: virStorageBackend virStorageBackendSCSI = { .type = VIR_STORAGE_POOL_SCSI, + .checkPool = virStorageBackendSCSICheckPool, .refreshPool = virStorageBackendSCSIRefreshPool, }; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f6672d9..9912429 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,17 +68,29 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { for (i = 0 ; i < driver->pools.count ; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; + virStorageBackendPtr backend; + bool started = false; virStoragePoolObjLock(pool); - if (pool->autostart && - !virStoragePoolObjIsActive(pool)) { - virStorageBackendPtr backend; - if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - VIR_ERROR(_("Missing backend %d"), pool->def->type); - virStoragePoolObjUnlock(pool); - continue; - } + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { + VIR_ERROR(_("Missing backend %d"), pool->def->type); + virStoragePoolObjUnlock(pool); + continue; + } + if (backend->checkPool && + backend->checkPool(NULL, pool, &started) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), + pool->def->name, err ? err->message : + "no error message found"); + virStoragePoolObjUnlock(pool); + continue; + } + + if (!started && + pool->autostart && + !virStoragePoolObjIsActive(pool)) { if (backend->startPool && backend->startPool(NULL, pool) < 0) { virErrorPtr err = virGetLastError(); @@ -88,7 +100,10 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { virStoragePoolObjUnlock(pool); continue; } + started = true; + } + if (started) { if (backend->refreshPool(NULL, pool) < 0) { virErrorPtr err = virGetLastError(); if (backend->stopPool) -- 1.7.2.3

On 11/12/2010 09:22 AM, Daniel P. Berrange wrote: <sys/types.h> is generally redundant, now that POSIX 2008 requires most headers to be self-contained. General observation: using stat() for existence checks is generally slower than access(,F_OK), because the kernel has to do more work to populate the result buffer that you then ignore. While what you have works, maybe you should consider rewriting this to use access(). Why <sys/wait.h>? Missing _() around that last string. ACK to the overall idea, but you may want to respin this given my comments. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Fri, Nov 12, 2010 at 04:22:43PM +0000, Daniel P. Berrange wrote: None of the host/bus/target/LUN values are really stable, although for our purposes LUN is likely to be. Have you considered LUN$lun for the volume names? This is a great addition.

On 11/12/2010 09:22 AM, Daniel P. Berrange wrote: s/THe/The/ I don't know if you adequately answered the usage questions raised by others, but from the code review aspect, I hadn't seen an ack yet. s/uniqness/uniqueness/ All the more reason for me to get my virCommand patch cleaned up per comments and pushed. This patch seems better off to rebase on top of virCommand. VIR_FDOPEN (hmm, I really need to get that promised syntax-check going for fdopen/[f]close). s/uniquness/uniqueness/ (cute - two unique mis-spellings for the same word :) -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 11/12/2010 09:22 AM, Daniel P. Berrange wrote: ACK, and not dependent on the rest of the series, if you want to rebase this and push this early. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org