[libvirt] Misc improvements to the storage driver
This patch series started out as an attempt to fix the seriously painful problem with iSCSI targets automatically logging in whenever the network came online. In the process of fixing this alot of other cruft is cleaned up docs/schemas/storagepool.rng | 5 src/conf/storage_conf.c | 21 + src/conf/storage_conf.h | 1 src/storage/storage_backend.h | 3 src/storage/storage_backend_fs.c | 29 ++ src/storage/storage_backend_iscsi.c | 427 +++++++++++++++++++++++++--------- src/storage/storage_backend_logical.c | 24 + src/storage/storage_backend_mpath.c | 20 + src/storage/storage_backend_scsi.c | 96 +++++++ src/storage/storage_driver.c | 65 +++-- tools/virsh.c | 51 ++-- 11 files changed, 573 insertions(+), 169 deletions(-) Daniel
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 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
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 +++++
Missing corresponding docs/formatstorage.html.in change.
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/>
Is text really appropriate, when...
@@ -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) {
it looks like you insist on an integer instead? For that matter, should you do a range check that the port is < 0x10000? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Nov 12, 2010 at 10:21:16AM -0700, Eric Blake wrote:
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
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 +++++
Missing corresponding docs/formatstorage.html.in change.
As per the commit message, this is already documented, but not implemented !
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/>
Is text really appropriate, when...
I guess it should reference a port number
@@ -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) {
it looks like you insist on an integer instead? For that matter, should you do a range check that the port is < 0x10000?
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 :|
On 11/12/2010 11:36 AM, Daniel P. Berrange wrote:
On Fri, Nov 12, 2010 at 10:21:16AM -0700, Eric Blake wrote:
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
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 +++++
Missing corresponding docs/formatstorage.html.in change.
As per the commit message, this is already documented, but not implemented !
Good to know.
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/>
Is text really appropriate?
I guess it should reference a port number
I don't see this patch applied yet. ACK with that nit to the schema fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
On Fri, Nov 12, 2010 at 04:22:35PM +0000, Daniel P. Berrange wrote:
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.
What's your motivation for removing the struct and passing the parameters individually? It's clearly not wrong, but why generate the code churn if it's not required?
It also removes hardcoding of port 3260 in the portal address, instead using the XML value if any.
ACK
* 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Nov 12, 2010 at 12:39:57PM -0500, Dave Allan wrote:
On Fri, Nov 12, 2010 at 04:22:35PM +0000, Daniel P. Berrange wrote:
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.
What's your motivation for removing the struct and passing the parameters individually? It's clearly not wrong, but why generate the code churn if it's not required?
The storage pool discovery code in a following patch does not have any virStoragePoolObjPtr available, since its very purpose is to discover one ! 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 :|
On Fri, Nov 12, 2010 at 06:37:36PM +0000, Daniel P. Berrange wrote:
On Fri, Nov 12, 2010 at 12:39:57PM -0500, Dave Allan wrote:
On Fri, Nov 12, 2010 at 04:22:35PM +0000, Daniel P. Berrange wrote:
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.
What's your motivation for removing the struct and passing the parameters individually? It's clearly not wrong, but why generate the code churn if it's not required?
The storage pool discovery code in a following patch does not have any virStoragePoolObjPtr available, since its very purpose is to discover one !
LOL, indeed, that's a good reason. Dave
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:
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.
Is that worth reporting as a bug in the iscsi toolchain? At any rate, we need this patch whether or not that tool changes behavior to something more sane. However, I'm not sure this is ready for ack without answers to some questions first:
To stop this stupid behaviour, we need to run
iscsiadm --portal $PORTAL --target $TARGET --op update --name node.startup --value manual
+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) {
Up to you if you want to rebase this to use VIR_RESIZE_N (or VIR_EXPAND_N), or to just leave that to me as a subsequent followup patch (I'm already searching through the code base for other instances to convert; one more won't hurt me).
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*$"
\s and \S are GNU-isms, and regcomp() on other platforms will reject it; is this regex only used on Linux, or do we need to be portable to iscsi implementations on other platforms?
+ }; + 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"));
Should this message be about iscsiadm rather than lvs? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 20/11/2010, at 5:45 AM, Eric Blake <eblake@redhat.com> wrote:
\s and \S are GNU-isms, and regcomp() on other platforms will reject it; is this regex only used on Linux, or do we need to be portable to iscsi implementations on other platforms?
As a data point, there are other iSCSI implementations on Linux too, some pretty widely used in some segments. For example, people in the HPC arena, who generally use networking gear that's 10, 20, 40, or 120 Gb/s, use an implementation based upon Linux SCST. It uses different commands to manage, not iscsiadm, and treats LUN 0 differently (from memory).
On Fri, Nov 19, 2010 at 07:40:12PM -0500, jclift@redhat.com wrote:
On 20/11/2010, at 5:45 AM, Eric Blake <eblake@redhat.com> wrote:
\s and \S are GNU-isms, and regcomp() on other platforms will reject it; is this regex only used on Linux, or do we need to be portable to iscsi implementations on other platforms?
As a data point, there are other iSCSI implementations on Linux too, some pretty widely used in some segments. For example, people in the HPC arena, who generally use networking gear that's 10, 20, 40, or 120 Gb/s, use an implementation based upon Linux SCST. It uses different commands to manage, not iscsiadm, and treats LUN 0 differently (from memory).
The iSCSI backend is already complicated enough without worrying about multiple impls for Linux & the impl we use is accessible in a wider array of Linux distros than SCST. Daniel
On Fri, Nov 19, 2010 at 11:09:31AM -0700, Eric Blake wrote:
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
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.
Is that worth reporting as a bug in the iscsi toolchain? At any rate, we need this patch whether or not that tool changes behavior to something more sane. However, I'm not sure this is ready for ack without answers to some questions first:
I've already reported it as a bug and it went nowhere productive with the iSCSI maintainer claiming this insanity is a desirable feature.
+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) {
Up to you if you want to rebase this to use VIR_RESIZE_N (or VIR_EXPAND_N), or to just leave that to me as a subsequent followup patch (I'm already searching through the code base for other instances to convert; one more won't hurt me).
I'll let you change this, there are many more instances of this in the storage backends already.
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*$"
\s and \S are GNU-isms, and regcomp() on other platforms will reject it; is this regex only used on Linux, or do we need to be portable to iscsi implementations on other platforms?
The storage drivers are already full of this regex syntax so this isn't making it any less portable really.
+ }; + 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"));
Should this message be about iscsiadm rather than lvs?
Yep. Daniel
Since the previous patch added support for parsing the output of the 'sendtargets' command, it is now trivial to support the storage pool discovery API. Given a hostname and optional portnumber and initiator IQN, the code can return a full list of storage pool source docs, each one representing a iSCSI target. * src/storage/storage_backend_iscsi.c: Wire up target auto-discovery --- src/storage/storage_backend_iscsi.c | 68 +++++++++++++++++++++++++++++++++++ 1 files changed, 68 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 2a36527..a67a428 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -571,6 +571,73 @@ virStorageBackendISCSIScanTargets(const char *portal, } +static char * +virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *srcSpec, + unsigned int flags ATTRIBUTE_UNUSED) +{ + virStoragePoolSourcePtr source = NULL; + size_t ntargets = 0; + char **targets = NULL; + char *ret = NULL; + int i; + virStoragePoolSourceList list = { + .type = VIR_STORAGE_POOL_ISCSI, + .nsources = 0, + .sources = NULL + }; + char *portal = NULL; + + if (!(source = virStoragePoolDefParseSourceString(srcSpec, + list.type))) + return NULL; + + if (!(portal = virStorageBackendISCSIPortal(source))) + goto cleanup; + + VIR_ERROR("Got %s %s %d\n", source->initiator.iqn, source->host.name, source->host.port); + + if (virStorageBackendISCSIScanTargets(portal, + source->initiator.iqn, + &ntargets, &targets) < 0) + goto cleanup; + + if (VIR_ALLOC_N(list.sources, ntargets) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0 ; i < ntargets ; i++) { + if (VIR_ALLOC_N(list.sources[i].devices, 1) < 0) { + virReportOOMError(); + goto cleanup; + } + list.sources[i].host = source->host; + list.sources[i].initiator = source->initiator; + list.sources[i].ndevice = 1; + list.sources[i].devices[0].path = targets[i]; + list.nsources++; + } + + if (!(ret = virStoragePoolSourceListFormat(&list))) { + virReportOOMError(); + goto cleanup; + } + +cleanup: + if (list.sources) { + for (i = 0 ; i < ntargets ; i++) + VIR_FREE(list.sources[i].devices); + VIR_FREE(list.sources); + } + for (i = 0 ; i < ntargets ; i++) + VIR_FREE(targets[i]); + VIR_FREE(targets); + VIR_FREE(portal); + virStoragePoolSourceFree(source); + return ret; +} + static int virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) @@ -671,4 +738,5 @@ virStorageBackend virStorageBackendISCSI = { .startPool = virStorageBackendISCSIStartPool, .refreshPool = virStorageBackendISCSIRefreshPool, .stopPool = virStorageBackendISCSIStopPool, + .findPoolSources = virStorageBackendISCSIFindPoolSources, }; -- 1.7.2.3
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
Since the previous patch added support for parsing the output of the 'sendtargets' command, it is now trivial to support the storage pool discovery API.
Given a hostname and optional portnumber and initiator IQN, the code can return a full list of storage pool source docs, each one representing a iSCSI target.
* src/storage/storage_backend_iscsi.c: Wire up target auto-discovery ---
+static char * +virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *srcSpec, + unsigned int flags ATTRIBUTE_UNUSED) +{
Should this check that flags==0? ACK with that nit addressed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The code generating XML for storage pool source discovery is hardcoded to only allow a hostname and optional port number. Refactor this code to make it easier to add support for extra parameters. * tools/virsh.c: Refactor XML generator --- tools/virsh.c | 26 ++++++++++---------------- 1 files changed, 10 insertions(+), 16 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d15a8df..6fc1b47 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5767,7 +5767,6 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) if (host) { size_t hostlen = strlen(host); char *port = vshCommandOptString(cmd, "port", &found); - int ret; if (!found) { port = strrchr(host, ':'); if (port) { @@ -5777,23 +5776,18 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) port = NULL; } } - ret = port ? - virAsprintf(&srcSpec, - "<source><host name='%.*s' port='%s'/></source>", - (int)hostlen, host, port) : - virAsprintf(&srcSpec, - "<source><host name='%.*s'/></source>", - (int)hostlen, host); - if (ret < 0) { - switch (errno) { - case ENOMEM: - vshError(ctl, "%s", _("Out of memory")); - break; - default: - vshError(ctl, _("virAsprintf failed (errno %d)"), errno); - } + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferAddLit(&buf, "<source>\n"); + virBufferVSprintf(&buf, " <host name='%.*s'",(int)hostlen, host); + if (port) + virBufferVSprintf(&buf, " port='%s'", port); + virBufferAddLit(&buf, "/>\n"); + virBufferAddLit(&buf, "</source>\n"); + if (virBufferError(&buf)) { + vshError(ctl, "%s", _("Out of memory")); return FALSE; } + srcSpec = virBufferContentAndReset(&buf); } srcList = virConnectFindStoragePoolSources(ctl->conn, type, srcSpec, 0); -- 1.7.2.3
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
The code generating XML for storage pool source discovery is hardcoded to only allow a hostname and optional port number. Refactor this code to make it easier to add support for extra parameters.
* tools/virsh.c: Refactor XML generator --- tools/virsh.c | 26 ++++++++++---------------- 1 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d15a8df..6fc1b47 100644 --- a/tools/virsh.c
+ virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferAddLit(&buf, "<source>\n"); + virBufferVSprintf(&buf, " <host name='%.*s'",(int)hostlen, host); + if (port) + virBufferVSprintf(&buf, " port='%s'", port); + virBufferAddLit(&buf, "/>\n"); + virBufferAddLit(&buf, "</source>\n");
Should these two lines be combined into a single virBufferAddLit? Then again, if later patches are adding in new elements besides <host>, it makes sense to insert between these two lines. ACK either way. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Allow an iSCSI initiator IQN to be set with the XML for the find-storage-pool-sources-as virsh command * tools/virsh.c: Add iSCSI IQN support --- tools/virsh.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 6fc1b47..69a42e8 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5743,6 +5743,7 @@ static const vshCmdOptDef opts_find_storage_pool_sources_as[] = { N_("type of storage pool sources to find")}, {"host", VSH_OT_DATA, VSH_OFLAG_NONE, N_("optional host to query")}, {"port", VSH_OT_DATA, VSH_OFLAG_NONE, N_("optional port to query")}, + {"initiator", VSH_OT_DATA, VSH_OFLAG_NONE, N_("optional initiator IQN to use for query")}, {NULL, 0, 0, NULL} }; @@ -5752,6 +5753,7 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) char *type, *host; char *srcSpec = NULL; char *srcList; + char *initiator; int found; type = vshCommandOptString(cmd, "type", &found); @@ -5760,6 +5762,9 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) host = vshCommandOptString(cmd, "host", &found); if (!found) host = NULL; + initiator = vshCommandOptString(cmd, "initiator", &found); + if (!found) + initiator = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; @@ -5782,6 +5787,11 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) if (port) virBufferVSprintf(&buf, " port='%s'", port); virBufferAddLit(&buf, "/>\n"); + if (initiator) { + virBufferAddLit(&buf, " <initiator>\n"); + virBufferVSprintf(&buf, " <iqn name='%s'/>\n", initiator); + virBufferAddLit(&buf, " </initiator>\n"); + } virBufferAddLit(&buf, "</source>\n"); if (virBufferError(&buf)) { vshError(ctl, "%s", _("Out of memory")); -- 1.7.2.3
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
Allow an iSCSI initiator IQN to be set with the XML for the find-storage-pool-sources-as virsh command
* tools/virsh.c: Add iSCSI IQN support --- tools/virsh.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
@@ -5782,6 +5787,11 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) if (port) virBufferVSprintf(&buf, " port='%s'", port); virBufferAddLit(&buf, "/>\n"); + if (initiator) { + virBufferAddLit(&buf, " <initiator>\n"); + virBufferVSprintf(&buf, " <iqn name='%s'/>\n", initiator); + virBufferAddLit(&buf, " </initiator>\n"); + }
Yep, answering my own question on patch 5/11, you stuck something in the middle. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The "find-storage-pool-sources-as" command takes two arguments, a hostname and a port number. For some reason the code would also then look for a port number appended to the hostname string by searching for ':'. This totally breaks if the user gives an IPv6 address, and is redundant, since you can already provide a port as a separate argument * tools/virsh.c: Remove bogus port number handling code --- tools/virsh.c | 11 ++--------- 1 files changed, 2 insertions(+), 9 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 69a42e8..a840758 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5772,15 +5772,8 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) if (host) { size_t hostlen = strlen(host); char *port = vshCommandOptString(cmd, "port", &found); - if (!found) { - port = strrchr(host, ':'); - if (port) { - if (*(++port)) - hostlen = port - host - 1; - else - port = NULL; - } - } + if (!found) + port = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "<source>\n"); virBufferVSprintf(&buf, " <host name='%.*s'",(int)hostlen, host); -- 1.7.2.3
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
The "find-storage-pool-sources-as" command takes two arguments, a hostname and a port number. For some reason the code would also then look for a port number appended to the hostname string by searching for ':'. This totally breaks if the user gives an IPv6 address, and is redundant, since you can already provide a port as a separate argument
* tools/virsh.c: Remove bogus port number handling code --- tools/virsh.c | 11 ++--------- 1 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 69a42e8..a840758 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5772,15 +5772,8 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const vshCmd * cmd ATTRIBUTE_UNUSED) if (host) { size_t hostlen = strlen(host); char *port = vshCommandOptString(cmd, "port", &found); - if (!found) { - port = strrchr(host, ':'); - if (port) { - if (*(++port)) - hostlen = port - host - 1; - else - port = NULL; - } - } + if (!found) + port = NULL;
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.
virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "<source>\n"); virBufferVSprintf(&buf, " <host name='%.*s'",(int)hostlen, host);
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Nov 19, 2010 at 11:23:00AM -0700, Eric Blake wrote:
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
The "find-storage-pool-sources-as" command takes two arguments, a hostname and a port number. For some reason the code would also then look for a port number appended to the hostname string by searching for ':'. This totally breaks if the user gives an IPv6 address, and is redundant, since you can already provide a port as a separate argument
* tools/virsh.c: Remove bogus port number handling code --- tools/virsh.c | 11 ++--------- 1 files changed, 2 insertions(+), 9 deletions(-)
virBuffer buf = VIR_BUFFER_INITIALIZER; virBufferAddLit(&buf, "<source>\n"); virBufferVSprintf(&buf, " <host name='%.*s'",(int)hostlen, host);
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.
Yep I'll kill that. Daniel
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 05:22 PM, Daniel P. Berrange wrote:
src/storage/storage_backend_mpath.c | 20 +++++++++++++++++++ src/storage/storage_backend_scsi.c | 25 ++++++++++++++++++++++++
Not related to your patch, but these two drivers are undocumented. Paolo
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
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.
+++ 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>
<sys/types.h> is generally redundant, now that POSIX 2008 requires most headers to be self-contained.
+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;
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().
+++ 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>
Why <sys/wait.h>?
+++ 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");
Missing _() around that last string. ACK to the overall idea, but you may want to respin this given my comments. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Many operations are not valid on inactive storage pools. The storage driver is currently returning VIR_ERR_INTERNAL_ERROR in these cases, rather than the more suitable error code VIR_ERR_OPERATION_INVALID * src/storage/storage_driver.c: Fix error code when pool is not active --- src/storage/storage_driver.c | 34 +++++++++++++++++----------------- 1 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9912429..f96068e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -627,7 +627,7 @@ storagePoolUndefine(virStoragePoolPtr obj) { } if (virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("pool is still active")); goto cleanup; } @@ -684,7 +684,7 @@ storagePoolStart(virStoragePoolPtr obj, goto cleanup; if (virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("pool already active")); goto cleanup; } @@ -729,7 +729,7 @@ storagePoolBuild(virStoragePoolPtr obj, goto cleanup; if (virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is already active")); goto cleanup; } @@ -766,7 +766,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { goto cleanup; if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -822,7 +822,7 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; if (virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is still active")); goto cleanup; } @@ -871,7 +871,7 @@ storagePoolRefresh(virStoragePoolPtr obj, goto cleanup; if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1076,7 +1076,7 @@ storagePoolNumVolumes(virStoragePoolPtr obj) { } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1109,7 +1109,7 @@ storagePoolListVolumes(virStoragePoolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1154,7 +1154,7 @@ storageVolumeLookupByName(virStoragePoolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1285,7 +1285,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1413,13 +1413,13 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } if (origpool && !virStoragePoolObjIsActive(origpool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1708,7 +1708,7 @@ storageVolumeWipe(virStorageVolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto out; } @@ -1765,7 +1765,7 @@ storageVolumeDelete(virStorageVolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1844,7 +1844,7 @@ storageVolumeGetInfo(virStorageVolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1897,7 +1897,7 @@ storageVolumeGetXMLDesc(virStorageVolPtr obj, } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } @@ -1944,7 +1944,7 @@ storageVolumeGetPath(virStorageVolPtr obj) { } if (!virStoragePoolObjIsActive(pool)) { - virStorageReportError(VIR_ERR_INTERNAL_ERROR, + virStorageReportError(VIR_ERR_OPERATION_INVALID, "%s", _("storage pool is not active")); goto cleanup; } -- 1.7.2.3
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
Many operations are not valid on inactive storage pools. The storage driver is currently returning VIR_ERR_INTERNAL_ERROR in these cases, rather than the more suitable error code VIR_ERR_OPERATION_INVALID
* src/storage/storage_driver.c: Fix error code when pool is not active
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The SCSI volumes currently get a name like '17:0:0:1' based on $host:$bus:$target:$lun. The names are intended to be unique per pool and stable across pool restarts. The inclusion of the $host component breaks this, because the $host number for iSCSI pools is dynamically allocated by the kernel at time of login. This changes the name to be 'unit:0:0:1', ie removes the leading host component. THe 'unit:' prefix is just to ensure the volume name doesn't start with a number and make it clearer when seen out of context. The SCSI volumes also get a 'key' field based on the fully qualified volume path. All SCSI volumes have a unique serial available in hardware which can be obtained by sending a suitable SCSI command. Call out to udev's 'scsi_id' command to fetch this value * src/storage/storage_backend_scsi.c: Improve key and name field value stability and uniqness --- src/storage/storage_backend_scsi.c | 71 +++++++++++++++++++++++++++++++++--- 1 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 0d6b1ac..aeabd36 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -163,9 +163,66 @@ cleanup: return ret; } +static char * +virStorageBackendSCSISerial(const char *dev) +{ + const char *cmdargv[] = { + "/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", dev, + NULL + }; + int fd = -1; + pid_t child; + FILE *list = NULL; + char line[1024]; + char *serial = NULL; + int err; + int exitstatus; + + /* Run the program and capture its output */ + if (virExec(cmdargv, NULL, NULL, + &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) + goto cleanup; + + if ((list = fdopen(fd, "r")) == NULL) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot read fd")); + goto cleanup; + } + + if (fgets(line, sizeof(line), list)) { + char *nl = strchr(line, '\n'); + if (nl) + *nl = '\0'; + VIR_ERROR("GOT ID %s\n", line); + serial = strdup(line); + } else { + VIR_ERROR("NO ID %s\n", dev); + serial = strdup(dev); + } + + if (!serial) { + virReportOOMError(); + goto cleanup; + } + +cleanup: + if (list) + fclose(list); + else + VIR_FORCE_CLOSE(fd); + + while ((err = waitpid(child, &exitstatus, 0) == -1) && errno == EINTR); + + return serial; +} + + static int virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, - uint32_t host, + uint32_t host ATTRIBUTE_UNUSED, uint32_t bus, uint32_t target, uint32_t lun, @@ -183,7 +240,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, vol->type = VIR_STORAGE_VOL_BLOCK; - if (virAsprintf(&(vol->name), "%u.%u.%u.%u", host, bus, target, lun) < 0) { + /* 'host' is dynamically allocated by the kernel, first come, + * first served, per HBA. As such it isn't suitable for use + * in the volume name. We only need uniquness per-pool, so + * just leave 'host' out + */ + if (virAsprintf(&(vol->name), "unit:%u:%u:%u", bus, target, lun) < 0) { virReportOOMError(); retval = -1; goto free_vol; @@ -231,10 +293,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; } - /* XXX should use logical unit's UUID instead */ - vol->key = strdup(vol->target.path); - if (vol->key == NULL) { - virReportOOMError(); + if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) { retval = -1; goto free_vol; } -- 1.7.2.3
On Fri, Nov 12, 2010 at 04:22:43PM +0000, Daniel P. Berrange wrote:
The SCSI volumes currently get a name like '17:0:0:1' based on $host:$bus:$target:$lun. The names are intended to be unique per pool and stable across pool restarts. The inclusion of the $host component breaks this, because the $host number for iSCSI pools is dynamically allocated by the kernel at time of login. This changes the name to be 'unit:0:0:1', ie removes the leading host component. THe 'unit:' prefix is just to ensure the volume name doesn't start with a number and make it clearer when seen out of context.
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?
The SCSI volumes also get a 'key' field based on the fully qualified volume path. All SCSI volumes have a unique serial available in hardware which can be obtained by sending a suitable SCSI command. Call out to udev's 'scsi_id' command to fetch this value
This is a great addition.
* src/storage/storage_backend_scsi.c: Improve key and name field value stability and uniqness --- src/storage/storage_backend_scsi.c | 71 +++++++++++++++++++++++++++++++++--- 1 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 0d6b1ac..aeabd36 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -163,9 +163,66 @@ cleanup: return ret; }
+static char * +virStorageBackendSCSISerial(const char *dev) +{ + const char *cmdargv[] = { + "/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", dev, + NULL + }; + int fd = -1; + pid_t child; + FILE *list = NULL; + char line[1024]; + char *serial = NULL; + int err; + int exitstatus; + + /* Run the program and capture its output */ + if (virExec(cmdargv, NULL, NULL, + &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) + goto cleanup; + + if ((list = fdopen(fd, "r")) == NULL) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot read fd")); + goto cleanup; + } + + if (fgets(line, sizeof(line), list)) { + char *nl = strchr(line, '\n'); + if (nl) + *nl = '\0'; + VIR_ERROR("GOT ID %s\n", line); + serial = strdup(line); + } else { + VIR_ERROR("NO ID %s\n", dev); + serial = strdup(dev); + } + + if (!serial) { + virReportOOMError(); + goto cleanup; + } + +cleanup: + if (list) + fclose(list); + else + VIR_FORCE_CLOSE(fd); + + while ((err = waitpid(child, &exitstatus, 0) == -1) && errno == EINTR); + + return serial; +} + + static int virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, - uint32_t host, + uint32_t host ATTRIBUTE_UNUSED, uint32_t bus, uint32_t target, uint32_t lun, @@ -183,7 +240,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
vol->type = VIR_STORAGE_VOL_BLOCK;
- if (virAsprintf(&(vol->name), "%u.%u.%u.%u", host, bus, target, lun) < 0) { + /* 'host' is dynamically allocated by the kernel, first come, + * first served, per HBA. As such it isn't suitable for use + * in the volume name. We only need uniquness per-pool, so + * just leave 'host' out + */ + if (virAsprintf(&(vol->name), "unit:%u:%u:%u", bus, target, lun) < 0) { virReportOOMError(); retval = -1; goto free_vol; @@ -231,10 +293,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; }
- /* XXX should use logical unit's UUID instead */ - vol->key = strdup(vol->target.path); - if (vol->key == NULL) { - virReportOOMError(); + if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) { retval = -1; goto free_vol; } -- 1.7.2.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Nov 12, 2010 at 12:37:41PM -0500, Dave Allan wrote:
On Fri, Nov 12, 2010 at 04:22:43PM +0000, Daniel P. Berrange wrote:
The SCSI volumes currently get a name like '17:0:0:1' based on $host:$bus:$target:$lun. The names are intended to be unique per pool and stable across pool restarts. The inclusion of the $host component breaks this, because the $host number for iSCSI pools is dynamically allocated by the kernel at time of login. This changes the name to be 'unit:0:0:1', ie removes the leading host component. THe 'unit:' prefix is just to ensure the volume name doesn't start with a number and make it clearer when seen out of context.
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?
The LUN$lun value isn't unique within the SCSI host, because the host can have multiple bus+target each with the same LUN value NB, "stable" depends on the context. For storage pools the requirement is that for a given hardware configuration, the name is stable every time the storage pool is started. This patch guarantees that, eg if you repeat this sequence of 6 commands twice in a row you should get the same results: virsh pool-start foo virsh vol-list foo virsh pool-destroy foo Previously you would not, for iSCSI or NPIV HBAs, now you will. If the admin actually plugs/unplugs physical units on the SCSI host, then obviously bus/target/lun values can all change, whcih is fine. If you're concerned about that problem in an app, then you should be using 'key' instead of 'name', which is permanently stable for the lifetime of the unit. Regards, 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 :|
On Fri, Nov 12, 2010 at 06:43:39PM +0000, Daniel P. Berrange wrote:
On Fri, Nov 12, 2010 at 12:37:41PM -0500, Dave Allan wrote:
On Fri, Nov 12, 2010 at 04:22:43PM +0000, Daniel P. Berrange wrote:
The SCSI volumes currently get a name like '17:0:0:1' based on $host:$bus:$target:$lun. The names are intended to be unique per pool and stable across pool restarts. The inclusion of the $host component breaks this, because the $host number for iSCSI pools is dynamically allocated by the kernel at time of login. This changes the name to be 'unit:0:0:1', ie removes the leading host component. THe 'unit:' prefix is just to ensure the volume name doesn't start with a number and make it clearer when seen out of context.
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?
The LUN$lun value isn't unique within the SCSI host, because the host can have multiple bus+target each with the same LUN value
Oops, how did that happen, somehow I got pool == target in my mind for a moment. Sigh.
NB, "stable" depends on the context. For storage pools the requirement is that for a given hardware configuration, the name is stable every time the storage pool is started. This patch guarantees that, eg if you repeat this sequence of 6 commands twice in a row you should get the same results:
virsh pool-start foo virsh vol-list foo virsh pool-destroy foo
Previously you would not, for iSCSI or NPIV HBAs, now you will.
If there is really a requirement that volume names remain stable, you can't base names on bus/target/lun; it's all based on order of discovery by the kernel and can change across reboots as a result of SAN topology changes that I would be hard pressed to characterize as a hardware change. I'm not saying it's a problem, after all, the way I named volumes they changed names every time an iSCSI pool restarted, but you should be aware of the possibility.
If the admin actually plugs/unplugs physical units on the SCSI host, then obviously bus/target/lun values can all change, whcih is fine. If you're concerned about that problem in an app, then you should be using 'key' instead of 'name', which is permanently stable for the lifetime of the unit.
You are assuming that the admin of the host is the same as the admin of the storage which is not generally the case with SANs. In any case, we should strongly encourage applications to use key if we don't already. Dave
On Fri, Nov 12, 2010 at 12:37:41PM -0500, Dave Allan wrote:
On Fri, Nov 12, 2010 at 04:22:43PM +0000, Daniel P. Berrange wrote:
The SCSI volumes also get a 'key' field based on the fully qualified volume path. All SCSI volumes have a unique serial available in hardware which can be obtained by sending a suitable SCSI command. Call out to udev's 'scsi_id' command to fetch this value
This is a great addition.
Oh I forgot to mention the other huge benefit of this..... With the key now based on the SCSI unit serial string, you can actually directly correlate the volumes libvirt shows you, with the volumes that your SAN/NAS shows you without having to guess or make assumptions on ordering. Regards, 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 :|
On 11/12/2010 05:22 PM, Daniel P. Berrange wrote:
+{ + const char *cmdargv[] = { + "/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", dev, + NULL + };
Does this needs to be "/lib/udev/scsi_id -gud <dev>" for RHEL5 compatibility? However, I can only get "scsi_id -gus /block/sda" to work on my RHEL5 installation, not "scsi_id -gud /dev/sda". Paolo
On Fri, Nov 12, 2010 at 07:32:05PM +0100, Paolo Bonzini wrote:
On 11/12/2010 05:22 PM, Daniel P. Berrange wrote:
+{ + const char *cmdargv[] = { + "/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", dev, + NULL + };
Does this needs to be "/lib/udev/scsi_id -gud <dev>" for RHEL5 compatibility? However, I can only get "scsi_id -gus /block/sda" to work on my RHEL5 installation, not "scsi_id -gud /dev/sda".
I haven't checked RHEL5. If we can't rely on scsi_id to work the same way across udev releases, then we could just pick a version of the code and copy it into libvirt tree, stripping it down to only the function we actually need. It isn't that much code and doing that's probably easier than playing games with a tool that changes its calling conventions 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 :|
On 11/12/2010 07:45 PM, Daniel P. Berrange wrote:
On Fri, Nov 12, 2010 at 07:32:05PM +0100, Paolo Bonzini wrote:
On 11/12/2010 05:22 PM, Daniel P. Berrange wrote:
+{ + const char *cmdargv[] = { + "/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", dev, + NULL + };
Does this needs to be "/lib/udev/scsi_id -gud<dev>" for RHEL5 compatibility? However, I can only get "scsi_id -gus /block/sda" to work on my RHEL5 installation, not "scsi_id -gud /dev/sda".
I haven't checked RHEL5. If we can't rely on scsi_id to work the same way across udev releases, then we could just pick a version of the code and copy it into libvirt tree, stripping it down to only the function we actually need. It isn't that much code and doing that's probably easier than playing games with a tool that changes its calling conventions
With the caveat that it is GPLv2, not LGPLv2. I haven't tested RHEL6 udev on RHEL5, also. Paolo
On Fri, Nov 12, 2010 at 08:06:32PM +0100, Paolo Bonzini wrote:
On 11/12/2010 07:45 PM, Daniel P. Berrange wrote:
On Fri, Nov 12, 2010 at 07:32:05PM +0100, Paolo Bonzini wrote:
On 11/12/2010 05:22 PM, Daniel P. Berrange wrote:
+{ + const char *cmdargv[] = { + "/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", dev, + NULL + };
Does this needs to be "/lib/udev/scsi_id -gud<dev>" for RHEL5 compatibility? However, I can only get "scsi_id -gus /block/sda" to work on my RHEL5 installation, not "scsi_id -gud /dev/sda".
I haven't checked RHEL5. If we can't rely on scsi_id to work the same way across udev releases, then we could just pick a version of the code and copy it into libvirt tree, stripping it down to only the function we actually need. It isn't that much code and doing that's probably easier than playing games with a tool that changes its calling conventions
With the caveat that it is GPLv2, not LGPLv2.
Yep, not a showstopper - we'd just do what we did with parted and set it up as a helper program as /usr/libexec/libvirt_scsi_id Regards, 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 :|
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
The SCSI volumes currently get a name like '17:0:0:1' based on $host:$bus:$target:$lun. The names are intended to be unique per pool and stable across pool restarts. The inclusion of the $host component breaks this, because the $host number for iSCSI pools is dynamically allocated by the kernel at time of login. This changes the name to be 'unit:0:0:1', ie removes the leading host component. THe 'unit:' prefix is just to ensure the volume
s/THe/The/
name doesn't start with a number and make it clearer when seen out of context.
The SCSI volumes also get a 'key' field based on the fully qualified volume path. All SCSI volumes have a unique serial available in hardware which can be obtained by sending a suitable SCSI command. Call out to udev's 'scsi_id' command to fetch this value
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.
* src/storage/storage_backend_scsi.c: Improve key and name field value stability and uniqness
s/uniqness/uniqueness/
+static char * +virStorageBackendSCSISerial(const char *dev) +{ + const char *cmdargv[] = { + "/lib/udev/scsi_id", + "--replace-whitespace", + "--whitelisted", + "--device", dev, + NULL + }; + int fd = -1; + pid_t child; + FILE *list = NULL; + char line[1024]; + char *serial = NULL; + int err; + int exitstatus; + + /* Run the program and capture its output */ + if (virExec(cmdargv, NULL, NULL, + &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) + goto cleanup;
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.
+ + if ((list = fdopen(fd, "r")) == NULL) {
VIR_FDOPEN (hmm, I really need to get that promised syntax-check going for fdopen/[f]close).
static int virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, - uint32_t host, + uint32_t host ATTRIBUTE_UNUSED, uint32_t bus, uint32_t target, uint32_t lun, @@ -183,7 +240,12 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
vol->type = VIR_STORAGE_VOL_BLOCK;
- if (virAsprintf(&(vol->name), "%u.%u.%u.%u", host, bus, target, lun) < 0) { + /* 'host' is dynamically allocated by the kernel, first come, + * first served, per HBA. As such it isn't suitable for use + * in the volume name. We only need uniquness per-pool, so
s/uniquness/uniqueness/ (cute - two unique mis-spellings for the same word :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
virsh was not checking for a error code when listing storage volumes. So when listing volumes in a pool that was shutoff, no output was displayed * tools/virsh.c: Fix error handling when listing volumes --- tools/virsh.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index a840758..49dcd6e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6738,6 +6738,12 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* Determine the number of volumes in the pool */ numVolumes = virStoragePoolNumOfVolumes(pool); + if (numVolumes < 0) { + vshError(ctl, "%s", _("Failed to list storage volumes")); + virStoragePoolFree(pool); + return FALSE; + } + /* Retrieve the list of volume names in the pool */ if (numVolumes > 0) { activeNames = vshCalloc(ctl, numVolumes, sizeof(*activeNames)); -- 1.7.2.3
On 11/12/2010 09:22 AM, Daniel P. Berrange wrote:
virsh was not checking for a error code when listing storage volumes. So when listing volumes in a pool that was shutoff, no output was displayed
* tools/virsh.c: Fix error handling when listing volumes --- tools/virsh.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index a840758..49dcd6e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6738,6 +6738,12 @@ cmdVolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) /* Determine the number of volumes in the pool */ numVolumes = virStoragePoolNumOfVolumes(pool);
+ if (numVolumes < 0) { + vshError(ctl, "%s", _("Failed to list storage volumes")); + virStoragePoolFree(pool); + return FALSE; + } +
ACK, and not dependent on the rest of the series, if you want to rebase this and push this early. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Nov 12, 2010 at 04:22:33PM +0000, Daniel P. Berrange wrote:
This patch series started out as an attempt to fix the seriously painful problem with iSCSI targets automatically logging in whenever the network came online. In the process of fixing this alot of other cruft is cleaned up
I've pushed this series with the fixes noted, except for the patch which calls out to udev's scsi_id command. I've removed that patch, until I can re-write it to be more portable across udev versions. Daniel
participants (5)
-
Daniel P. Berrange -
Dave Allan -
Eric Blake -
jclift＠redhat.com -
Paolo Bonzini