[libvirt] [PATCHv2 0/8] Test parsing of iscsiadm output

v1: https://www.redhat.com/archives/libvir-list/2014-March/msg00720.html v2: * instead of using cat, switch RunProgRegex to use a synchronous command and modify the output buffer via a virCommandDryRun callback * move the functions dealing with iscsiadm out of the iscsi backend to src/util (and move StorageBackendRunRegex as well) Ján Tomko (8): Sort includes in storage_backend_iscsi.c Move virStorageBackendRun to vircommand Switch virCommandRunRegex to use virStringSplit Don't create iscsiadm command line in ISCSIPool{Start,Stop} Remove storage pool from the arguments of a few functions Move functions using iscsiadm to viriscsi.c Add test for virISCSIGetSession Add test for virISCSIScanTargets po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 11 + src/storage/storage_backend.c | 249 ----------------- src/storage/storage_backend.h | 22 -- src/storage/storage_backend_disk.c | 43 +-- src/storage/storage_backend_fs.c | 9 +- src/storage/storage_backend_iscsi.c | 482 ++------------------------------ src/storage/storage_backend_iscsi.h | 4 - src/storage/storage_backend_logical.c | 63 +++-- src/util/vircommand.c | 242 +++++++++++++++++ src/util/vircommand.h | 20 ++ src/util/viriscsi.c | 498 ++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 52 ++++ tests/Makefile.am | 6 + tests/viriscsitest.c | 219 +++++++++++++++ 16 files changed, 1145 insertions(+), 777 deletions(-) create mode 100644 src/util/viriscsi.c create mode 100644 src/util/viriscsi.h create mode 100644 tests/viriscsitest.c -- 1.8.3.2

--- src/storage/storage_backend_iscsi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 2a4e669..0feeb5f 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -34,13 +34,13 @@ #include "datatypes.h" #include "driver.h" -#include "virerror.h" #include "storage_backend_scsi.h" #include "storage_backend_iscsi.h" #include "viralloc.h" -#include "virlog.h" -#include "virfile.h" #include "vircommand.h" +#include "virerror.h" +#include "virfile.h" +#include "virlog.h" #include "virobject.h" #include "virrandom.h" #include "virstring.h" -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:26PM +0100, Ján Tomko wrote:
--- src/storage/storage_backend_iscsi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 2a4e669..0feeb5f 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -34,13 +34,13 @@
#include "datatypes.h" #include "driver.h" -#include "virerror.h" #include "storage_backend_scsi.h" #include "storage_backend_iscsi.h" #include "viralloc.h" -#include "virlog.h" -#include "virfile.h" #include "vircommand.h" +#include "virerror.h" +#include "virfile.h" +#include "virlog.h" #include "virobject.h" #include "virrandom.h" #include "virstring.h"
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

The only storage-specific parameter is the pool object, which is only used for passing to the callback function. --- src/libvirt_private.syms | 2 + src/storage/storage_backend.c | 249 ---------------------------------- src/storage/storage_backend.h | 22 --- src/storage/storage_backend_disk.c | 43 +++--- src/storage/storage_backend_fs.c | 9 +- src/storage/storage_backend_iscsi.c | 54 ++++---- src/storage/storage_backend_logical.c | 63 +++++---- src/util/vircommand.c | 245 +++++++++++++++++++++++++++++++++ src/util/vircommand.h | 20 +++ 9 files changed, 360 insertions(+), 347 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3baf766..c7e024d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1113,6 +1113,8 @@ virCommandRawStatus; virCommandRequireHandshake; virCommandRun; virCommandRunAsync; +virCommandRunNul; +virCommandRunRegex; virCommandSetAppArmorProfile; virCommandSetDryRun; virCommandSetErrorBuffer; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d14e633..b1421ec 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1632,252 +1632,3 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, return stablepath; } - - -#ifndef WIN32 -/* - * Run an external program. - * - * Read its output and apply a series of regexes to each line - * When the entire set of regexes has matched consecutively - * then run a callback passing in all the matches - */ -int -virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, - virCommandPtr cmd, - int nregex, - const char **regex, - int *nvars, - virStorageBackendListVolRegexFunc func, - void *data, const char *prefix) -{ - int fd = -1, err, ret = -1; - FILE *list = NULL; - regex_t *reg; - regmatch_t *vars = NULL; - char line[1024]; - int maxReg = 0; - size_t i, j; - int totgroups = 0, ngroup = 0, maxvars = 0; - char **groups; - - /* Compile all regular expressions */ - if (VIR_ALLOC_N(reg, nregex) < 0) - return -1; - - for (i = 0; i < nregex; i++) { - err = regcomp(®[i], regex[i], REG_EXTENDED); - if (err != 0) { - char error[100]; - regerror(err, ®[i], error, sizeof(error)); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), error); - for (j = 0; j < i; j++) - regfree(®[j]); - VIR_FREE(reg); - return -1; - } - - totgroups += nvars[i]; - if (nvars[i] > maxvars) - maxvars = nvars[i]; - - } - - /* Storage for matched variables */ - if (VIR_ALLOC_N(groups, totgroups) < 0) - goto cleanup; - if (VIR_ALLOC_N(vars, maxvars+1) < 0) - goto cleanup; - - virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) { - goto cleanup; - } - - if ((list = VIR_FDOPEN(fd, "r")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot read fd")); - goto cleanup; - } - - while (fgets(line, sizeof(line), list) != NULL) { - char *p = NULL; - /* Strip trailing newline */ - int len = strlen(line); - if (len && line[len-1] == '\n') - line[len-1] = '\0'; - - /* ignore any command prefix */ - if (prefix) - p = STRSKIP(line, prefix); - if (!p) - p = line; - - for (i = 0; i <= maxReg && i < nregex; i++) { - if (regexec(®[i], p, nvars[i]+1, vars, 0) == 0) { - maxReg++; - - if (i == 0) - ngroup = 0; - - /* NULL terminate each captured group in the line */ - for (j = 0; j < nvars[i]; j++) { - /* NB vars[0] is the full pattern, so we offset j by 1 */ - p[vars[j+1].rm_eo] = '\0'; - if (VIR_STRDUP(groups[ngroup++], p + vars[j+1].rm_so) < 0) - goto cleanup; - } - - /* We're matching on the last regex, so callback time */ - if (i == (nregex-1)) { - if (((*func)(pool, groups, data)) < 0) - goto cleanup; - - /* Release matches & restart to matching the first regex */ - for (j = 0; j < totgroups; j++) - VIR_FREE(groups[j]); - maxReg = 0; - ngroup = 0; - } - } - } - } - - ret = virCommandWait(cmd, NULL); -cleanup: - if (groups) { - for (j = 0; j < totgroups; j++) - VIR_FREE(groups[j]); - VIR_FREE(groups); - } - VIR_FREE(vars); - - for (i = 0; i < nregex; i++) - regfree(®[i]); - - VIR_FREE(reg); - - VIR_FORCE_FCLOSE(list); - VIR_FORCE_CLOSE(fd); - - return ret; -} - -/* - * Run an external program and read from its standard output - * a stream of tokens from IN_STREAM, applying FUNC to - * each successive sequence of N_COLUMNS tokens. - * If FUNC returns < 0, stop processing input and return -1. - * Return -1 if N_COLUMNS == 0. - * Return -1 upon memory allocation error. - * If the number of input tokens is not a multiple of N_COLUMNS, - * then the final FUNC call will specify a number smaller than N_COLUMNS. - * If there are no input tokens (empty input), call FUNC with N_COLUMNS == 0. - */ -int -virStorageBackendRunProgNul(virStoragePoolObjPtr pool, - virCommandPtr cmd, - size_t n_columns, - virStorageBackendListVolNulFunc func, - void *data) -{ - size_t n_tok = 0; - int fd = -1; - FILE *fp = NULL; - char **v; - int ret = -1; - size_t i; - - if (n_columns == 0) - return -1; - - if (VIR_ALLOC_N(v, n_columns) < 0) - return -1; - for (i = 0; i < n_columns; i++) - v[i] = NULL; - - virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) { - goto cleanup; - } - - if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot open file using fd")); - goto cleanup; - } - - while (1) { - char *buf = NULL; - size_t buf_len = 0; - /* Be careful: even when it returns -1, - this use of getdelim allocates memory. */ - ssize_t tok_len = getdelim(&buf, &buf_len, 0, fp); - v[n_tok] = buf; - if (tok_len < 0) { - /* Maybe EOF, maybe an error. - If n_tok > 0, then we know it's an error. */ - if (n_tok && func(pool, n_tok, v, data) < 0) - goto cleanup; - break; - } - ++n_tok; - if (n_tok == n_columns) { - if (func(pool, n_tok, v, data) < 0) - goto cleanup; - n_tok = 0; - for (i = 0; i < n_columns; i++) { - VIR_FREE(v[i]); - } - } - } - - if (feof(fp) < 0) { - virReportSystemError(errno, "%s", - _("read error on pipe")); - goto cleanup; - } - - ret = virCommandWait(cmd, NULL); - cleanup: - for (i = 0; i < n_columns; i++) - VIR_FREE(v[i]); - VIR_FREE(v); - - VIR_FORCE_FCLOSE(fp); - VIR_FORCE_CLOSE(fd); - - return ret; -} - -#else /* WIN32 */ - -int -virStorageBackendRunProgRegex(virConnectPtr conn, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - const char *const*prog ATTRIBUTE_UNUSED, - int nregex ATTRIBUTE_UNUSED, - const char **regex ATTRIBUTE_UNUSED, - int *nvars ATTRIBUTE_UNUSED, - virStorageBackendListVolRegexFunc func ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) -{ - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s not implemented on Win32"), __FUNCTION__); - return -1; -} - -int -virStorageBackendRunProgNul(virConnectPtr conn, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - const char **prog ATTRIBUTE_UNUSED, - size_t n_columns ATTRIBUTE_UNUSED, - virStorageBackendListVolNulFunc func ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) -{ - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s not implemented on Win32"), __FUNCTION__); - return -1; -} -#endif /* WIN32 */ diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 5314411..aaa17a0 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -159,28 +159,6 @@ char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, bool loop); -typedef int (*virStorageBackendListVolRegexFunc)(virStoragePoolObjPtr pool, - char **const groups, - void *data); -typedef int (*virStorageBackendListVolNulFunc)(virStoragePoolObjPtr pool, - size_t n_tokens, - char **const groups, - void *data); - -int virStorageBackendRunProgRegex(virStoragePoolObjPtr pool, - virCommandPtr cmd, - int nregex, - const char **regex, - int *nvars, - virStorageBackendListVolRegexFunc func, - void *data, const char *cmd_to_ignore); - -int virStorageBackendRunProgNul(virStoragePoolObjPtr pool, - virCommandPtr cmd, - size_t n_columns, - virStorageBackendListVolNulFunc func, - void *data); - virCommandPtr virStorageBackendCreateQemuImgCmd(virConnectPtr conn, virStoragePoolObjPtr pool, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 05799a3..81201fd 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -188,12 +188,18 @@ virStorageBackendDiskMakeFreeExtent(virStoragePoolObjPtr pool, } +struct virStorageBackendDiskPoolVolData { + virStoragePoolObjPtr pool; + virStorageVolDefPtr vol; +}; + static int -virStorageBackendDiskMakeVol(virStoragePoolObjPtr pool, - size_t ntok ATTRIBUTE_UNUSED, +virStorageBackendDiskMakeVol(size_t ntok ATTRIBUTE_UNUSED, char **const groups, - void *data) + void *opaque) { + struct virStorageBackendDiskPoolVolData *data = opaque; + virStoragePoolObjPtr pool = data->pool; /* * Ignore normal+metadata, and logical+metadata partitions * since they're basically internal book-keeping regions @@ -209,7 +215,7 @@ virStorageBackendDiskMakeVol(virStoragePoolObjPtr pool, /* Remaining data / metadata parts get turn into volumes... */ if (STREQ(groups[2], "metadata") || STREQ(groups[2], "data")) { - virStorageVolDefPtr vol = data; + virStorageVolDefPtr vol = data->vol; if (vol) { /* We're searching for a specific vol only */ @@ -234,7 +240,6 @@ virStorageBackendDiskMakeVol(virStoragePoolObjPtr pool, } } - /* To get a list of partitions we run an external helper * tool which then uses parted APIs. This is because * parted's API is not compatible with libvirt's license @@ -259,25 +264,28 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool, virCommandPtr cmd = virCommandNewArgList(PARTHELPER, pool->def->source.devices[0].path, NULL); + struct virStorageBackendDiskPoolVolData cbdata = { + .pool = pool, + .vol = vol, + }; int ret; pool->def->allocation = pool->def->capacity = pool->def->available = 0; - ret = virStorageBackendRunProgNul(pool, - cmd, - 6, - virStorageBackendDiskMakeVol, - vol); + ret = virCommandRunNul(cmd, + 6, + virStorageBackendDiskMakeVol, + &cbdata); virCommandFree(cmd); return ret; } static int -virStorageBackendDiskMakePoolGeometry(virStoragePoolObjPtr pool, - size_t ntok ATTRIBUTE_UNUSED, +virStorageBackendDiskMakePoolGeometry(size_t ntok ATTRIBUTE_UNUSED, char **const groups, - void *data ATTRIBUTE_UNUSED) + void *data) { + virStoragePoolObjPtr pool = data; virStoragePoolSourceDevicePtr device = &(pool->def->source.devices[0]); if (virStrToLong_i(groups[0], NULL, 0, &device->geometry.cylinders) < 0 || virStrToLong_i(groups[1], NULL, 0, &device->geometry.heads) < 0 || @@ -299,11 +307,10 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) NULL); int ret; - ret = virStorageBackendRunProgNul(pool, - cmd, - 3, - virStorageBackendDiskMakePoolGeometry, - NULL); + ret = virCommandRunNul(cmd, + 3, + virStorageBackendDiskMakePoolGeometry, + pool); virCommandFree(cmd); return ret; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index edb7cd0..b6fed01 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -202,8 +202,7 @@ struct _virNetfsDiscoverState { typedef struct _virNetfsDiscoverState virNetfsDiscoverState; static int -virStorageBackendFileSystemNetFindPoolSourcesFunc(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - char **const groups, +virStorageBackendFileSystemNetFindPoolSourcesFunc(char **const groups, void *data) { virNetfsDiscoverState *state = data; @@ -301,9 +300,9 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE source->hosts[0].name, NULL); - if (virStorageBackendRunProgRegex(NULL, cmd, 1, regexes, vars, - virStorageBackendFileSystemNetFindPoolSourcesFunc, - &state, NULL) < 0) + if (virCommandRunRegex(cmd, 1, regexes, vars, + virStorageBackendFileSystemNetFindPoolSourcesFunc, + &state, NULL) < 0) goto cleanup; retval = virStoragePoolSourceListFormat(&state.list); diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 0feeb5f..20fc0e6 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -79,16 +79,19 @@ virStorageBackendISCSIPortal(virStoragePoolSourcePtr source) return portal; } +struct virStorageBackendISCSISessionData { + char *session; + const char *devpath; +}; static int -virStorageBackendISCSIExtractSession(virStoragePoolObjPtr pool, - char **const groups, - void *data) +virStorageBackendISCSIExtractSession(char **const groups, + void *opaque) { - char **session = data; + struct virStorageBackendISCSISessionData *data = opaque; - if (STREQ(groups[1], pool->def->source.devices[0].path)) - return VIR_STRDUP(*session, groups[0]); + if (STREQ(groups[1], data->devpath)) + return VIR_STRDUP(data->session, groups[0]); return 0; } @@ -109,21 +112,22 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, int vars[] = { 2, }; - char *session = NULL; + struct virStorageBackendISCSISessionData cbdata = { + .session = NULL, + .devpath = pool->def->source.devices[0].path + }; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); - if (virStorageBackendRunProgRegex(pool, - cmd, - 1, - regexes, - vars, - virStorageBackendISCSIExtractSession, - &session, NULL) < 0) + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virStorageBackendISCSIExtractSession, + &cbdata, NULL) < 0) goto cleanup; - if (session == NULL && - !probe) { + if (cbdata.session == NULL && !probe) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot find session")); goto cleanup; @@ -131,7 +135,7 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, cleanup: virCommandFree(cmd); - return session; + return cbdata.session; } @@ -439,8 +443,7 @@ struct virStorageBackendISCSITargetList { }; static int -virStorageBackendISCSIGetTargets(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - char **const groups, +virStorageBackendISCSIGetTargets(char **const groups, void *data) { struct virStorageBackendISCSITargetList *list = data; @@ -503,13 +506,12 @@ virStorageBackendISCSIScanTargets(const char *portal, memset(&list, 0, sizeof(list)); - if (virStorageBackendRunProgRegex(NULL, /* No pool for callback */ - cmd, - 1, - regexes, - vars, - virStorageBackendISCSIGetTargets, - &list, NULL) < 0) + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virStorageBackendISCSIGetTargets, + &list, NULL) < 0) goto cleanup; for (i = 0; i < list.ntargets; i++) { diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 667fb06..907b9b0 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -66,11 +66,17 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" +struct virStorageBackendLogicalPoolVolData { + virStoragePoolObjPtr pool; + virStorageVolDefPtr vol; +}; + static int -virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, - char **const groups, - void *data) +virStorageBackendLogicalMakeVol(char **const groups, + void *opaque) { + struct virStorageBackendLogicalPoolVolData *data = opaque; + virStoragePoolObjPtr pool = data->pool; virStorageVolDefPtr vol = NULL; bool is_new_vol = false; unsigned long long offset, size, length; @@ -96,8 +102,8 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, return 0; /* See if we're only looking for a specific volume */ - if (data != NULL) { - vol = data; + if (data->vol != NULL) { + vol = data->vol; if (STRNEQ(vol->name, groups[0])) return 0; } @@ -299,6 +305,10 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, }; int ret = -1; virCommandPtr cmd; + struct virStorageBackendLogicalPoolVolData cbdata = { + .pool = pool, + .vol = vol, + }; cmd = virCommandNewArgList(LVS, "--separator", "#", @@ -310,13 +320,13 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool, "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr", pool->def->source.name, NULL); - if (virStorageBackendRunProgRegex(pool, - cmd, - 1, - regexes, - vars, - virStorageBackendLogicalMakeVol, - vol, "lvs") < 0) + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virStorageBackendLogicalMakeVol, + &cbdata, + "lvs") < 0) goto cleanup; ret = 0; @@ -326,10 +336,10 @@ cleanup: } static int -virStorageBackendLogicalRefreshPoolFunc(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - char **const groups, - void *data ATTRIBUTE_UNUSED) +virStorageBackendLogicalRefreshPoolFunc(char **const groups, + void *data) { + virStoragePoolObjPtr pool = data; if (virStrToLong_ull(groups[0], NULL, 10, &pool->def->capacity) < 0) return -1; if (virStrToLong_ull(groups[1], NULL, 10, &pool->def->available) < 0) @@ -341,8 +351,7 @@ virStorageBackendLogicalRefreshPoolFunc(virStoragePoolObjPtr pool ATTRIBUTE_UNUS static int -virStorageBackendLogicalFindPoolSourcesFunc(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - char **const groups, +virStorageBackendLogicalFindPoolSourcesFunc(char **const groups, void *data) { virStoragePoolSourceListPtr sourceList = data; @@ -432,9 +441,9 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, "--noheadings", "-o", "pv_name,vg_name", NULL); - if (virStorageBackendRunProgRegex(NULL, cmd, 1, regexes, vars, - virStorageBackendLogicalFindPoolSourcesFunc, - &sourceList, "pvs") < 0) { + if (virCommandRunRegex(cmd, 1, regexes, vars, + virStorageBackendLogicalFindPoolSourcesFunc, + &sourceList, "pvs") < 0) { virCommandFree(cmd); return NULL; } @@ -593,13 +602,13 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, NULL); /* Now get basic volgrp metadata */ - if (virStorageBackendRunProgRegex(pool, - cmd, - 1, - regexes, - vars, - virStorageBackendLogicalRefreshPoolFunc, - NULL, "vgs") < 0) + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virStorageBackendLogicalRefreshPoolFunc, + pool, + "vgs") < 0) goto cleanup; ret = 0; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 79bb20c..3f98eb8 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -22,6 +22,7 @@ #include <config.h> #include <poll.h> +#include <regex.h> #include <signal.h> #include <stdarg.h> #include <stdlib.h> @@ -2760,3 +2761,247 @@ virCommandSetDryRun(virBufferPtr buf, dryRunCallback = cb; dryRunOpaque = opaque; } + +#ifndef WIN32 +/* + * Run an external program. + * + * Read its output and apply a series of regexes to each line + * When the entire set of regexes has matched consecutively + * then run a callback passing in all the matches + */ +int +virCommandRunRegex(virCommandPtr cmd, + int nregex, + const char **regex, + int *nvars, + virCommandRunRegexFunc func, + void *data, + const char *prefix) +{ + int fd = -1, err, ret = -1; + FILE *list = NULL; + regex_t *reg; + regmatch_t *vars = NULL; + char line[1024]; + int maxReg = 0; + size_t i, j; + int totgroups = 0, ngroup = 0, maxvars = 0; + char **groups; + + /* Compile all regular expressions */ + if (VIR_ALLOC_N(reg, nregex) < 0) + return -1; + + for (i = 0; i < nregex; i++) { + err = regcomp(®[i], regex[i], REG_EXTENDED); + if (err != 0) { + char error[100]; + regerror(err, ®[i], error, sizeof(error)); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to compile regex %s"), error); + for (j = 0; j < i; j++) + regfree(®[j]); + VIR_FREE(reg); + return -1; + } + + totgroups += nvars[i]; + if (nvars[i] > maxvars) + maxvars = nvars[i]; + + } + + /* Storage for matched variables */ + if (VIR_ALLOC_N(groups, totgroups) < 0) + goto cleanup; + if (VIR_ALLOC_N(vars, maxvars+1) < 0) + goto cleanup; + + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { + goto cleanup; + } + + if ((list = VIR_FDOPEN(fd, "r")) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot read fd")); + goto cleanup; + } + + while (fgets(line, sizeof(line), list) != NULL) { + char *p = NULL; + /* Strip trailing newline */ + int len = strlen(line); + if (len && line[len-1] == '\n') + line[len-1] = '\0'; + + /* ignore any command prefix */ + if (prefix) + p = STRSKIP(line, prefix); + if (!p) + p = line; + + for (i = 0; i <= maxReg && i < nregex; i++) { + if (regexec(®[i], p, nvars[i]+1, vars, 0) == 0) { + maxReg++; + + if (i == 0) + ngroup = 0; + + /* NULL terminate each captured group in the line */ + for (j = 0; j < nvars[i]; j++) { + /* NB vars[0] is the full pattern, so we offset j by 1 */ + p[vars[j+1].rm_eo] = '\0'; + if (VIR_STRDUP(groups[ngroup++], p + vars[j+1].rm_so) < 0) + goto cleanup; + } + + /* We're matching on the last regex, so callback time */ + if (i == (nregex-1)) { + if (((*func)(groups, data)) < 0) + goto cleanup; + + /* Release matches & restart to matching the first regex */ + for (j = 0; j < totgroups; j++) + VIR_FREE(groups[j]); + maxReg = 0; + ngroup = 0; + } + } + } + } + + ret = virCommandWait(cmd, NULL); +cleanup: + if (groups) { + for (j = 0; j < totgroups; j++) + VIR_FREE(groups[j]); + VIR_FREE(groups); + } + VIR_FREE(vars); + + for (i = 0; i < nregex; i++) + regfree(®[i]); + + VIR_FREE(reg); + + VIR_FORCE_FCLOSE(list); + VIR_FORCE_CLOSE(fd); + + return ret; +} + +/* + * Run an external program and read from its standard output + * a stream of tokens from IN_STREAM, applying FUNC to + * each successive sequence of N_COLUMNS tokens. + * If FUNC returns < 0, stop processing input and return -1. + * Return -1 if N_COLUMNS == 0. + * Return -1 upon memory allocation error. + * If the number of input tokens is not a multiple of N_COLUMNS, + * then the final FUNC call will specify a number smaller than N_COLUMNS. + * If there are no input tokens (empty input), call FUNC with N_COLUMNS == 0. + */ +int +virCommandRunNul(virCommandPtr cmd, + size_t n_columns, + virCommandRunNulFunc func, + void *data) +{ + size_t n_tok = 0; + int fd = -1; + FILE *fp = NULL; + char **v; + int ret = -1; + size_t i; + + if (n_columns == 0) + return -1; + + if (VIR_ALLOC_N(v, n_columns) < 0) + return -1; + for (i = 0; i < n_columns; i++) + v[i] = NULL; + + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { + goto cleanup; + } + + if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot open file using fd")); + goto cleanup; + } + + while (1) { + char *buf = NULL; + size_t buf_len = 0; + /* Be careful: even when it returns -1, + this use of getdelim allocates memory. */ + ssize_t tok_len = getdelim(&buf, &buf_len, 0, fp); + v[n_tok] = buf; + if (tok_len < 0) { + /* Maybe EOF, maybe an error. + If n_tok > 0, then we know it's an error. */ + if (n_tok && func(n_tok, v, data) < 0) + goto cleanup; + break; + } + ++n_tok; + if (n_tok == n_columns) { + if (func(n_tok, v, data) < 0) + goto cleanup; + n_tok = 0; + for (i = 0; i < n_columns; i++) { + VIR_FREE(v[i]); + } + } + } + + if (feof(fp) < 0) { + virReportSystemError(errno, "%s", + _("read error on pipe")); + goto cleanup; + } + + ret = virCommandWait(cmd, NULL); + cleanup: + for (i = 0; i < n_columns; i++) + VIR_FREE(v[i]); + VIR_FREE(v); + + VIR_FORCE_FCLOSE(fp); + VIR_FORCE_CLOSE(fd); + + return ret; +} + +#else /* WIN32 */ + +int +virCommandRunRegex(virCommandPtr cmd ATTRIBUTE_UNUSED, + int nregex ATTRIBUTE_UNUSED, + const char **regex ATTRIBUTE_UNUSED, + int *nvars ATTRIBUTE_UNUSED, + virCommandRunRegexFunc func ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + const char *prefix ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s not implemented on Win32"), __FUNCTION__); + return -1; +} + +int +virCommandRunNul(virCommandPtr cmd ATTRIBUTE_UNUSED, + size_t n_columns ATTRIBUTE_UNUSED, + virCommandRunNulFunc func ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s not implemented on Win32"), __FUNCTION__); + return -1; +} +#endif /* WIN32 */ diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 929375b..8cdb31c 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -187,4 +187,24 @@ void virCommandFree(virCommandPtr cmd); void virCommandDoAsyncIO(virCommandPtr cmd); +typedef int (*virCommandRunRegexFunc)(char **const groups, + void *data); +typedef int (*virCommandRunNulFunc)(size_t n_tokens, + char **const groups, + void *data); + +int virCommandRunRegex(virCommandPtr cmd, + int nregex, + const char **regex, + int *nvars, + virCommandRunRegexFunc func, + void *data, + const char *cmd_to_ignore); + +int virCommandRunNul(virCommandPtr cmd, + size_t n_columns, + virCommandRunNulFunc func, + void *data); + + #endif /* __VIR_COMMAND_H__ */ -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:27PM +0100, Ján Tomko wrote:
The only storage-specific parameter is the pool object, which is only used for passing to the callback function. --- src/libvirt_private.syms | 2 + src/storage/storage_backend.c | 249 ---------------------------------- src/storage/storage_backend.h | 22 --- src/storage/storage_backend_disk.c | 43 +++--- src/storage/storage_backend_fs.c | 9 +- src/storage/storage_backend_iscsi.c | 54 ++++---- src/storage/storage_backend_logical.c | 63 +++++---- src/util/vircommand.c | 245 +++++++++++++++++++++++++++++++++ src/util/vircommand.h | 20 +++ 9 files changed, 360 insertions(+), 347 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Instead of running the command asynchronously and reading the output via fgets, let virCommand collect the output and split it with virStringSplit. --- src/util/vircommand.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 3f98eb8..8250634 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2779,15 +2779,16 @@ virCommandRunRegex(virCommandPtr cmd, void *data, const char *prefix) { - int fd = -1, err, ret = -1; - FILE *list = NULL; + int err; regex_t *reg; regmatch_t *vars = NULL; - char line[1024]; int maxReg = 0; - size_t i, j; + size_t i, j, k; int totgroups = 0, ngroup = 0, maxvars = 0; char **groups; + char *outbuf = NULL; + char **lines = NULL; + int ret = -1; /* Compile all regular expressions */ if (VIR_ALLOC_N(reg, nregex) < 0) @@ -2818,29 +2819,27 @@ virCommandRunRegex(virCommandPtr cmd, if (VIR_ALLOC_N(vars, maxvars+1) < 0) goto cleanup; - virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) { + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; - } - if ((list = VIR_FDOPEN(fd, "r")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot read fd")); + if (!outbuf) { + /* no output */ + ret = 0; goto cleanup; } - while (fgets(line, sizeof(line), list) != NULL) { + if (!(lines = virStringSplit(outbuf, "\n", 0))) + goto cleanup; + + for (k = 0; lines[k]; k++) { char *p = NULL; - /* Strip trailing newline */ - int len = strlen(line); - if (len && line[len-1] == '\n') - line[len-1] = '\0'; /* ignore any command prefix */ if (prefix) - p = STRSKIP(line, prefix); + p = STRSKIP(lines[k], prefix); if (!p) - p = line; + p = lines[k]; for (i = 0; i <= maxReg && i < nregex; i++) { if (regexec(®[i], p, nvars[i]+1, vars, 0) == 0) { @@ -2872,8 +2871,10 @@ virCommandRunRegex(virCommandPtr cmd, } } - ret = virCommandWait(cmd, NULL); + ret = 0; cleanup: + virStringFreeList(lines); + VIR_FREE(outbuf); if (groups) { for (j = 0; j < totgroups; j++) VIR_FREE(groups[j]); @@ -2885,10 +2886,6 @@ cleanup: regfree(®[i]); VIR_FREE(reg); - - VIR_FORCE_FCLOSE(list); - VIR_FORCE_CLOSE(fd); - return ret; } -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:28PM +0100, Ján Tomko wrote:
Instead of running the command asynchronously and reading the output via fgets, let virCommand collect the output and split it with virStringSplit. --- src/util/vircommand.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Create ISCSIConnection{Login,Logout} wrappers for that. --- src/storage/storage_backend_iscsi.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 20fc0e6..35c9cc5 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -356,6 +356,24 @@ cleanup: } static int +virStorageBackendISCSIConnectionLogin(const char *portal, + const char *initiatoriqn, + const char *target) +{ + const char *extraargv[] = { "--login", NULL }; + return virStorageBackendISCSIConnection(portal, initiatoriqn, target, extraargv); +} + +static int +virStorageBackendISCSIConnectionLogout(const char *portal, + const char *initiatoriqn, + const char *target) +{ + const char *extraargv[] = { "--logout", NULL }; + return virStorageBackendISCSIConnection(portal, initiatoriqn, target, extraargv); +} + +static int virStorageBackendISCSIGetHostNumber(const char *sysfs_path, uint32_t *host) { @@ -789,7 +807,6 @@ virStorageBackendISCSIStartPool(virConnectPtr conn, char *portal = NULL; char *session = NULL; int ret = -1; - const char *loginargv[] = { "--login", NULL }; if (pool->def->source.nhost != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -825,10 +842,9 @@ virStorageBackendISCSIStartPool(virConnectPtr conn, if (virStorageBackendISCSISetAuth(portal, conn, pool->def) < 0) goto cleanup; - if (virStorageBackendISCSIConnection(portal, - pool->def->source.initiator.iqn, - pool->def->source.devices[0].path, - loginargv) < 0) + if (virStorageBackendISCSIConnectionLogin(portal, + pool->def->source.initiator.iqn, + pool->def->source.devices[0].path) < 0) goto cleanup; } ret = 0; @@ -867,17 +883,15 @@ static int virStorageBackendISCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - const char *logoutargv[] = { "--logout", NULL }; char *portal; int ret = -1; if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) return -1; - if (virStorageBackendISCSIConnection(portal, - pool->def->source.initiator.iqn, - pool->def->source.devices[0].path, - logoutargv) < 0) + if (virStorageBackendISCSIConnectionLogout(portal, + pool->def->source.initiator.iqn, + pool->def->source.devices[0].path) < 0) goto cleanup; ret = 0; -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:29PM +0100, Ján Tomko wrote:
Create ISCSIConnection{Login,Logout} wrappers for that. --- src/storage/storage_backend_iscsi.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

virStorageBackendISCSISession only needs the path of the source device and virStorageBackendISCSIRescanLUNs doesn't need the pool at all. This will allow the functions to be moved to src/util. --- src/storage/storage_backend_iscsi.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 35c9cc5..b7a0380 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -96,8 +96,8 @@ virStorageBackendISCSIExtractSession(char **const groups, } static char * -virStorageBackendISCSISession(virStoragePoolObjPtr pool, - bool probe) +virStorageBackendISCSIGetSession(const char *devpath, + bool probe) { /* * # iscsiadm --mode session @@ -114,7 +114,7 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, }; struct virStorageBackendISCSISessionData cbdata = { .session = NULL, - .devpath = pool->def->source.devices[0].path + .devpath = devpath, }; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); @@ -138,6 +138,13 @@ cleanup: return cbdata.session; } +static char * +virStorageBackendISCSISession(virStoragePoolObjPtr pool, + bool probe) +{ + return virStorageBackendISCSIGetSession(pool->def->source.devices[0].path, probe); +} + #define LINE_SIZE 4096 @@ -442,8 +449,7 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool, } static int -virStorageBackendISCSIRescanLUNs(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - const char *session) +virStorageBackendISCSIRescanLUNs(const char *session) { virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", @@ -865,7 +871,7 @@ virStorageBackendISCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, if ((session = virStorageBackendISCSISession(pool, false)) == NULL) goto cleanup; - if (virStorageBackendISCSIRescanLUNs(pool, session) < 0) + if (virStorageBackendISCSIRescanLUNs(session) < 0) goto cleanup; if (virStorageBackendISCSIFindLUs(pool, session) < 0) goto cleanup; -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:30PM +0100, Ján Tomko wrote:
virStorageBackendISCSISession only needs the path of the source device and virStorageBackendISCSIRescanLUNs doesn't need the pool at all.
This will allow the functions to be moved to src/util. --- src/storage/storage_backend_iscsi.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Remove the 'StorageBackend' from names of the functions and fix indentation. --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 9 + src/storage/storage_backend_iscsi.c | 498 ++---------------------------------- src/storage/storage_backend_iscsi.h | 4 - src/util/viriscsi.c | 498 ++++++++++++++++++++++++++++++++++++ src/util/viriscsi.h | 52 ++++ 7 files changed, 588 insertions(+), 475 deletions(-) create mode 100644 src/util/viriscsi.c create mode 100644 src/util/viriscsi.h diff --git a/po/POTFILES.in b/po/POTFILES.in index efac7b2..5a4112a 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -168,6 +168,7 @@ src/util/virhostdev.c src/util/viridentity.c src/util/virinitctl.c src/util/viriptables.c +src/util/viriscsi.c src/util/virjson.c src/util/virkeyfile.c src/util/virlockspace.c diff --git a/src/Makefile.am b/src/Makefile.am index 4fdd871..55427ed 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -113,6 +113,7 @@ UTIL_SOURCES = \ util/viridentity.c util/viridentity.h \ util/virinitctl.c util/virinitctl.h \ util/viriptables.c util/viriptables.h \ + util/viriscsi.c util/viriscsi.h \ util/virjson.c util/virjson.h \ util/virkeycode.c util/virkeycode.h \ util/virkeyfile.c util/virkeyfile.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c7e024d..d72a9af 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1359,6 +1359,15 @@ iptablesRemoveUdpInput; iptablesRemoveUdpOutput; +# util/viriscsi.h +virISCSIConnectionLogin; +virISCSIConnectionLogout; +virISCSIGetSession; +virISCSINodeUpdate; +virISCSIRescanLUNs; +virISCSIScanTargets; + + # util/virjson.h virJSONValueArrayAppend; virJSONValueArrayGet; diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index b7a0380..7e7ffad 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -26,8 +26,6 @@ #include <dirent.h> #include <sys/wait.h> #include <string.h> -#include <stdio.h> -#include <regex.h> #include <fcntl.h> #include <unistd.h> #include <sys/stat.h> @@ -40,9 +38,9 @@ #include "vircommand.h" #include "virerror.h" #include "virfile.h" +#include "viriscsi.h" #include "virlog.h" #include "virobject.h" -#include "virrandom.h" #include "virstring.h" #include "viruuid.h" @@ -79,306 +77,14 @@ virStorageBackendISCSIPortal(virStoragePoolSourcePtr source) return portal; } -struct virStorageBackendISCSISessionData { - char *session; - const char *devpath; -}; - -static int -virStorageBackendISCSIExtractSession(char **const groups, - void *opaque) -{ - struct virStorageBackendISCSISessionData *data = opaque; - - if (STREQ(groups[1], data->devpath)) - return VIR_STRDUP(data->session, groups[0]); - return 0; -} - -static char * -virStorageBackendISCSIGetSession(const char *devpath, - bool probe) -{ - /* - * # iscsiadm --mode session - * tcp: [1] 192.168.122.170:3260,1 demo-tgt-b - * tcp: [2] 192.168.122.170:3260,1 demo-tgt-a - * - * Pull out 2nd and 4th fields - */ - const char *regexes[] = { - "^tcp:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+).*$" - }; - int vars[] = { - 2, - }; - struct virStorageBackendISCSISessionData cbdata = { - .session = NULL, - .devpath = devpath, - }; - - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); - - if (virCommandRunRegex(cmd, - 1, - regexes, - vars, - virStorageBackendISCSIExtractSession, - &cbdata, NULL) < 0) - goto cleanup; - - if (cbdata.session == NULL && !probe) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot find session")); - goto cleanup; - } - -cleanup: - virCommandFree(cmd); - return cbdata.session; -} static char * virStorageBackendISCSISession(virStoragePoolObjPtr pool, bool probe) { - return virStorageBackendISCSIGetSession(pool->def->source.devices[0].path, probe); -} - - -#define LINE_SIZE 4096 - -static int -virStorageBackendIQNFound(const char *initiatoriqn, - char **ifacename) -{ - int ret = IQN_MISSING, fd = -1; - char ebuf[64]; - FILE *fp = NULL; - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, - "--mode", "iface", NULL); - - if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { - ret = IQN_ERROR; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not allocate memory for output of '%s'"), - ISCSIADM); - goto out; - } - - memset(line, 0, LINE_SIZE); - - virCommandSetOutputFD(cmd, &fd); - if (virCommandRunAsync(cmd, NULL) < 0) { - ret = IQN_ERROR; - goto out; - } - - if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to open stream for file descriptor " - "when reading output from '%s': '%s'"), - ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf))); - ret = IQN_ERROR; - goto out; - } - - while (fgets(line, LINE_SIZE, fp) != NULL) { - newline = strrchr(line, '\n'); - if (newline == NULL) { - ret = IQN_ERROR; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected line > %d characters " - "when parsing output of '%s'"), - LINE_SIZE, ISCSIADM); - goto out; - } - *newline = '\0'; - - iqn = strrchr(line, ','); - if (iqn == NULL) { - continue; - } - iqn++; - - if (STREQ(iqn, initiatoriqn)) { - token = strchr(line, ' '); - if (!token) { - ret = IQN_ERROR; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing space when parsing output " - "of '%s'"), ISCSIADM); - goto out; - } - if (VIR_STRNDUP(*ifacename, line, token - line) < 0) { - ret = IQN_ERROR; - goto out; - } - VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn); - ret = IQN_FOUND; - break; - } - } - - if (virCommandWait(cmd, NULL) < 0) - ret = IQN_ERROR; - -out: - if (ret == IQN_MISSING) { - VIR_DEBUG("Could not find interface with IQN '%s'", iqn); - } - - VIR_FREE(line); - VIR_FORCE_FCLOSE(fp); - VIR_FORCE_CLOSE(fd); - virCommandFree(cmd); - - return ret; -} - - -static int -virStorageBackendCreateIfaceIQN(const char *initiatoriqn, - char **ifacename) -{ - int ret = -1, exitstatus = -1; - char *temp_ifacename; - virCommandPtr cmd = NULL; - - if (virAsprintf(&temp_ifacename, - "libvirt-iface-%08llx", - (unsigned long long)virRandomBits(30)) < 0) - return -1; - - VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'", - temp_ifacename, initiatoriqn); - - cmd = virCommandNewArgList(ISCSIADM, - "--mode", "iface", - "--interface", temp_ifacename, - "--op", "new", - NULL); - /* Note that we ignore the exitstatus. Older versions of iscsiadm - * tools returned an exit status of > 0, even if they succeeded. - * We will just rely on whether the interface got created - * properly. */ - if (virCommandRun(cmd, &exitstatus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run command '%s' to create new iscsi interface"), - ISCSIADM); - goto cleanup; - } - virCommandFree(cmd); - - cmd = virCommandNewArgList(ISCSIADM, - "--mode", "iface", - "--interface", temp_ifacename, - "--op", "update", - "--name", "iface.initiatorname", - "--value", - initiatoriqn, - NULL); - /* Note that we ignore the exitstatus. Older versions of iscsiadm tools - * returned an exit status of > 0, even if they succeeded. We will just - * rely on whether iface file got updated properly. */ - if (virCommandRun(cmd, &exitstatus) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), - ISCSIADM, initiatoriqn); - goto cleanup; - } - - /* Check again to make sure the interface was created. */ - if (virStorageBackendIQNFound(initiatoriqn, ifacename) != IQN_FOUND) { - VIR_DEBUG("Failed to find interface '%s' with IQN '%s' " - "after attempting to create it", - &temp_ifacename[0], initiatoriqn); - goto cleanup; - } else { - VIR_DEBUG("Interface '%s' with IQN '%s' was created successfully", - *ifacename, initiatoriqn); - } - - ret = 0; - -cleanup: - virCommandFree(cmd); - VIR_FREE(temp_ifacename); - if (ret != 0) - VIR_FREE(*ifacename); - return ret; -} - - - -static int -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 - }; - virCommandPtr cmd; - char *ifacename = NULL; - - cmd = virCommandNewArgs(baseargv); - virCommandAddArgSet(cmd, extraargv); - - 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; - } - virCommandAddArgList(cmd, "--interface", ifacename, NULL); - } - - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - -cleanup: - virCommandFree(cmd); - VIR_FREE(ifacename); - - return ret; -} - -static int -virStorageBackendISCSIConnectionLogin(const char *portal, - const char *initiatoriqn, - const char *target) -{ - const char *extraargv[] = { "--login", NULL }; - return virStorageBackendISCSIConnection(portal, initiatoriqn, target, extraargv); + return virISCSIGetSession(pool->def->source.devices[0].path, probe); } -static int -virStorageBackendISCSIConnectionLogout(const char *portal, - const char *initiatoriqn, - const char *target) -{ - const char *extraargv[] = { "--logout", NULL }; - return virStorageBackendISCSIConnection(portal, initiatoriqn, target, extraargv); -} static int virStorageBackendISCSIGetHostNumber(const char *sysfs_path, @@ -448,124 +154,6 @@ virStorageBackendISCSIFindLUs(virStoragePoolObjPtr pool, return retval; } -static int -virStorageBackendISCSIRescanLUNs(const char *session) -{ - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, - "--mode", "session", - "-r", session, - "-R", - NULL); - int ret = virCommandRun(cmd, NULL); - virCommandFree(cmd); - return ret; -} - -struct virStorageBackendISCSITargetList { - size_t ntargets; - char **targets; -}; - -static int -virStorageBackendISCSIGetTargets(char **const groups, - void *data) -{ - struct virStorageBackendISCSITargetList *list = data; - char *target; - - if (VIR_STRDUP(target, groups[1]) < 0) - return -1; - - if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0) { - VIR_FREE(target); - return -1; - } - - 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, - const char *initiatoriqn, - size_t *ntargetsret, - char ***targetsret) -{ - /** - * - * 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 }; - struct virStorageBackendISCSITargetList list; - size_t i; - int ret = -1; - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, - "--mode", "discovery", - "--type", "sendtargets", - "--portal", portal, - NULL); - - memset(&list, 0, sizeof(list)); - - if (virCommandRunRegex(cmd, - 1, - regexes, - vars, - virStorageBackendISCSIGetTargets, - &list, NULL) < 0) - goto cleanup; - - 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); - } - - ret = 0; -cleanup: - virCommandFree(cmd); - return ret; -} - static char * virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -606,9 +194,9 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, if (!(portal = virStorageBackendISCSIPortal(source))) goto cleanup; - if (virStorageBackendISCSIScanTargets(portal, - source->initiator.iqn, - &ntargets, &targets) < 0) + if (virISCSIScanTargets(portal, + source->initiator.iqn, + &ntargets, &targets) < 0) goto cleanup; if (VIR_ALLOC_N(list.sources, ntargets) < 0) @@ -683,38 +271,6 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } -static int -virStorageBackendISCSINodeUpdate(const char *portal, - const char *target, - const char *name, - const char *value) -{ - virCommandPtr cmd = NULL; - int status; - int ret = -1; - - cmd = virCommandNewArgList(ISCSIADM, - "--mode", "node", - "--portal", portal, - "--target", target, - "--op", "update", - "--name", name, - "--value", value, - NULL); - - /* Ignore non-zero status. */ - if (virCommandRun(cmd, &status) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to update '%s' of node mode for target '%s'"), - name, target); - goto cleanup; - } - - ret = 0; -cleanup: - virCommandFree(cmd); - return ret; -} static int virStorageBackendISCSISetAuth(const char *portal, @@ -784,18 +340,18 @@ virStorageBackendISCSISetAuth(const char *portal, goto cleanup; } - if (virStorageBackendISCSINodeUpdate(portal, - def->source.devices[0].path, - "node.session.auth.authmethod", - "CHAP") < 0 || - virStorageBackendISCSINodeUpdate(portal, - def->source.devices[0].path, - "node.session.auth.username", - chap.username) < 0 || - virStorageBackendISCSINodeUpdate(portal, - def->source.devices[0].path, - "node.session.auth.password", - (const char *)secret_value) < 0) + if (virISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.password", + (const char *)secret_value) < 0) goto cleanup; ret = 0; @@ -840,17 +396,17 @@ virStorageBackendISCSIStartPool(virConnectPtr conn, * iscsiadm doesn't let you login to a target, unless you've * first issued a 'sendtargets' command to the portal :-( */ - if (virStorageBackendISCSIScanTargets(portal, - pool->def->source.initiator.iqn, - NULL, NULL) < 0) + if (virISCSIScanTargets(portal, + pool->def->source.initiator.iqn, + NULL, NULL) < 0) goto cleanup; if (virStorageBackendISCSISetAuth(portal, conn, pool->def) < 0) goto cleanup; - if (virStorageBackendISCSIConnectionLogin(portal, - pool->def->source.initiator.iqn, - pool->def->source.devices[0].path) < 0) + if (virISCSIConnectionLogin(portal, + pool->def->source.initiator.iqn, + pool->def->source.devices[0].path) < 0) goto cleanup; } ret = 0; @@ -871,7 +427,7 @@ virStorageBackendISCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, if ((session = virStorageBackendISCSISession(pool, false)) == NULL) goto cleanup; - if (virStorageBackendISCSIRescanLUNs(session) < 0) + if (virISCSIRescanLUNs(session) < 0) goto cleanup; if (virStorageBackendISCSIFindLUs(pool, session) < 0) goto cleanup; @@ -895,9 +451,9 @@ virStorageBackendISCSIStopPool(virConnectPtr conn ATTRIBUTE_UNUSED, if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) return -1; - if (virStorageBackendISCSIConnectionLogout(portal, - pool->def->source.initiator.iqn, - pool->def->source.devices[0].path) < 0) + if (virISCSIConnectionLogout(portal, + pool->def->source.initiator.iqn, + pool->def->source.devices[0].path) < 0) goto cleanup; ret = 0; diff --git a/src/storage/storage_backend_iscsi.h b/src/storage/storage_backend_iscsi.h index 910a795..da3b22c 100644 --- a/src/storage/storage_backend_iscsi.h +++ b/src/storage/storage_backend_iscsi.h @@ -28,8 +28,4 @@ extern virStorageBackend virStorageBackendISCSI; -# define IQN_FOUND 1 -# define IQN_MISSING 0 -# define IQN_ERROR -1 - #endif /* __VIR_STORAGE_BACKEND_ISCSI_H__ */ diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c new file mode 100644 index 0000000..18a595f --- /dev/null +++ b/src/util/viriscsi.c @@ -0,0 +1,498 @@ +/* + * viriscsi.c: helper APIs for managing iSCSI + * + * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <regex.h> +#include <stdio.h> + +#include "viriscsi.h" + +#include "viralloc.h" +#include "vircommand.h" +#include "virerror.h" +#include "virfile.h" +#include "virlog.h" +#include "virrandom.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.iscsi"); + + +struct virISCSISessionData { + char *session; + const char *devpath; +}; + + +static int +virISCSIExtractSession(char **const groups, + void *opaque) +{ + struct virISCSISessionData *data = opaque; + + if (STREQ(groups[1], data->devpath)) + return VIR_STRDUP(data->session, groups[0]); + return 0; +} + + +char * +virISCSIGetSession(const char *devpath, + bool probe) +{ + /* + * # iscsiadm --mode session + * tcp: [1] 192.168.122.170:3260,1 demo-tgt-b + * tcp: [2] 192.168.122.170:3260,1 demo-tgt-a + * + * Pull out 2nd and 4th fields + */ + const char *regexes[] = { + "^tcp:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+).*$" + }; + int vars[] = { + 2, + }; + struct virISCSISessionData cbdata = { + .session = NULL, + .devpath = devpath, + }; + + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); + + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virISCSIExtractSession, + &cbdata, NULL) < 0) + goto cleanup; + + if (cbdata.session == NULL && !probe) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot find session")); + goto cleanup; + } + +cleanup: + virCommandFree(cmd); + return cbdata.session; +} + + + +#define LINE_SIZE 4096 +#define IQN_FOUND 1 +#define IQN_MISSING 0 +#define IQN_ERROR -1 + +static int +virStorageBackendIQNFound(const char *initiatoriqn, + char **ifacename) +{ + int ret = IQN_MISSING, fd = -1; + char ebuf[64]; + FILE *fp = NULL; + char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, + "--mode", "iface", NULL); + + if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { + ret = IQN_ERROR; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not allocate memory for output of '%s'"), + ISCSIADM); + goto out; + } + + memset(line, 0, LINE_SIZE); + + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { + ret = IQN_ERROR; + goto out; + } + + if ((fp = VIR_FDOPEN(fd, "r")) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open stream for file descriptor " + "when reading output from '%s': '%s'"), + ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf))); + ret = IQN_ERROR; + goto out; + } + + while (fgets(line, LINE_SIZE, fp) != NULL) { + newline = strrchr(line, '\n'); + if (newline == NULL) { + ret = IQN_ERROR; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected line > %d characters " + "when parsing output of '%s'"), + LINE_SIZE, ISCSIADM); + goto out; + } + *newline = '\0'; + + iqn = strrchr(line, ','); + if (iqn == NULL) { + continue; + } + iqn++; + + if (STREQ(iqn, initiatoriqn)) { + token = strchr(line, ' '); + if (!token) { + ret = IQN_ERROR; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing space when parsing output " + "of '%s'"), ISCSIADM); + goto out; + } + if (VIR_STRNDUP(*ifacename, line, token - line) < 0) { + ret = IQN_ERROR; + goto out; + } + VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn); + ret = IQN_FOUND; + break; + } + } + + if (virCommandWait(cmd, NULL) < 0) + ret = IQN_ERROR; + +out: + if (ret == IQN_MISSING) { + VIR_DEBUG("Could not find interface with IQN '%s'", iqn); + } + + VIR_FREE(line); + VIR_FORCE_FCLOSE(fp); + VIR_FORCE_CLOSE(fd); + virCommandFree(cmd); + + return ret; +} + + +static int +virStorageBackendCreateIfaceIQN(const char *initiatoriqn, + char **ifacename) +{ + int ret = -1, exitstatus = -1; + char *temp_ifacename; + virCommandPtr cmd = NULL; + + if (virAsprintf(&temp_ifacename, + "libvirt-iface-%08llx", + (unsigned long long)virRandomBits(30)) < 0) + return -1; + + VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'", + temp_ifacename, initiatoriqn); + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "iface", + "--interface", temp_ifacename, + "--op", "new", + NULL); + /* Note that we ignore the exitstatus. Older versions of iscsiadm + * tools returned an exit status of > 0, even if they succeeded. + * We will just rely on whether the interface got created + * properly. */ + if (virCommandRun(cmd, &exitstatus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to run command '%s' to create new iscsi interface"), + ISCSIADM); + goto cleanup; + } + virCommandFree(cmd); + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "iface", + "--interface", temp_ifacename, + "--op", "update", + "--name", "iface.initiatorname", + "--value", + initiatoriqn, + NULL); + /* Note that we ignore the exitstatus. Older versions of iscsiadm tools + * returned an exit status of > 0, even if they succeeded. We will just + * rely on whether iface file got updated properly. */ + if (virCommandRun(cmd, &exitstatus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), + ISCSIADM, initiatoriqn); + goto cleanup; + } + + /* Check again to make sure the interface was created. */ + if (virStorageBackendIQNFound(initiatoriqn, ifacename) != IQN_FOUND) { + VIR_DEBUG("Failed to find interface '%s' with IQN '%s' " + "after attempting to create it", + &temp_ifacename[0], initiatoriqn); + goto cleanup; + } else { + VIR_DEBUG("Interface '%s' with IQN '%s' was created successfully", + *ifacename, initiatoriqn); + } + + ret = 0; + +cleanup: + virCommandFree(cmd); + VIR_FREE(temp_ifacename); + if (ret != 0) + VIR_FREE(*ifacename); + return ret; +} + + +static int +virISCSIConnection(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 + }; + virCommandPtr cmd; + char *ifacename = NULL; + + cmd = virCommandNewArgs(baseargv); + virCommandAddArgSet(cmd, extraargv); + + 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; + } + virCommandAddArgList(cmd, "--interface", ifacename, NULL); + } + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virCommandFree(cmd); + VIR_FREE(ifacename); + + return ret; +} + + +int +virISCSIConnectionLogin(const char *portal, + const char *initiatoriqn, + const char *target) +{ + const char *extraargv[] = { "--login", NULL }; + return virISCSIConnection(portal, initiatoriqn, target, extraargv); +} + + +int +virISCSIConnectionLogout(const char *portal, + const char *initiatoriqn, + const char *target) +{ + const char *extraargv[] = { "--logout", NULL }; + return virISCSIConnection(portal, initiatoriqn, target, extraargv); +} + + +int +virISCSIRescanLUNs(const char *session) +{ + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, + "--mode", "session", + "-r", session, + "-R", + NULL); + int ret = virCommandRun(cmd, NULL); + virCommandFree(cmd); + return ret; +} + + +struct virISCSITargetList { + size_t ntargets; + char **targets; +}; + + +static int +virISCSIGetTargets(char **const groups, + void *data) +{ + struct virISCSITargetList *list = data; + char *target; + + if (VIR_STRDUP(target, groups[1]) < 0) + return -1; + + if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0) { + VIR_FREE(target); + return -1; + } + + return 0; +} + + +static int +virISCSITargetAutologin(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 virISCSIConnection(portal, initiatoriqn, target, extraargv); +} + + +int +virISCSIScanTargets(const char *portal, + const char *initiatoriqn, + size_t *ntargetsret, + char ***targetsret) +{ + /** + * + * 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 }; + struct virISCSITargetList list; + size_t i; + int ret = -1; + virCommandPtr cmd = virCommandNewArgList(ISCSIADM, + "--mode", "discovery", + "--type", "sendtargets", + "--portal", portal, + NULL); + + memset(&list, 0, sizeof(list)); + + if (virCommandRunRegex(cmd, + 1, + regexes, + vars, + virISCSIGetTargets, + &list, NULL) < 0) + goto cleanup; + + 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 (virISCSITargetAutologin(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); + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +} + + +int +virISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value) +{ + virCommandPtr cmd = NULL; + int status; + int ret = -1; + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "node", + "--portal", portal, + "--target", target, + "--op", "update", + "--name", name, + "--value", value, + NULL); + + /* Ignore non-zero status. */ + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to update '%s' of node mode for target '%s'"), + name, target); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +} diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h new file mode 100644 index 0000000..462e56a --- /dev/null +++ b/src/util/viriscsi.h @@ -0,0 +1,52 @@ +/* + * viriscsi.h: helper APIs for managing iSCSI + * + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __VIR_ISCSI_H__ +# define __VIR_ISCSI_H__ + +# include "internal.h" + +char * +virISCSIGetSession(const char *devpath, + bool probe); + +int +virISCSIConnectionLogin(const char *portal, + const char *initiatoriqn, + const char *target); +int +virISCSIConnectionLogout(const char *portal, + const char *initiatoriqn, + const char *target); +int +virISCSIRescanLUNs(const char *session); + +int +virISCSIScanTargets(const char *portal, + const char *initiatoriqn, + size_t *ntargetsret, + char ***targetsret); +int +virISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value); +#endif -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:31PM +0100, Ján Tomko wrote:
diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h new file mode 100644 index 0000000..462e56a --- /dev/null +++ b/src/util/viriscsi.h @@ -0,0 +1,52 @@ +#ifndef __VIR_ISCSI_H__ +# define __VIR_ISCSI_H__ + +# include "internal.h" + +char * +virISCSIGetSession(const char *devpath, + bool probe);
Could add ATTRIBUTE_NONNULL for this 'const char *' and all the other pointers below
+ +int +virISCSIConnectionLogin(const char *portal, + const char *initiatoriqn, + const char *target); +int +virISCSIConnectionLogout(const char *portal, + const char *initiatoriqn, + const char *target); +int +virISCSIRescanLUNs(const char *session); + +int +virISCSIScanTargets(const char *portal, + const char *initiatoriqn, + size_t *ntargetsret, + char ***targetsret); +int +virISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value);
And these 5 could have ATTRIBUTE_RETURN_CHECK too Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Mar 19, 2014 at 04:35:31PM +0000, Daniel P. Berrange wrote:
On Wed, Mar 19, 2014 at 04:52:31PM +0100, Ján Tomko wrote:
diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h new file mode 100644 index 0000000..462e56a --- /dev/null +++ b/src/util/viriscsi.h @@ -0,0 +1,52 @@ +#ifndef __VIR_ISCSI_H__ +# define __VIR_ISCSI_H__ + +# include "internal.h" + +char * +virISCSIGetSession(const char *devpath, + bool probe);
Could add ATTRIBUTE_NONNULL for this 'const char *' and all the other pointers below
+ +int +virISCSIConnectionLogin(const char *portal, + const char *initiatoriqn, + const char *target); +int +virISCSIConnectionLogout(const char *portal, + const char *initiatoriqn, + const char *target); +int +virISCSIRescanLUNs(const char *session); + +int +virISCSIScanTargets(const char *portal, + const char *initiatoriqn, + size_t *ntargetsret, + char ***targetsret); +int +virISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value);
And these 5 could have ATTRIBUTE_RETURN_CHECK too
Btw, I meant to say ACK - you can add the attribute annotations without reposting this series. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Parse iscsiadm output with and without the recently introduced flashnode info. [1] This should check that commits like 57e17a7 (fixing [2]) do not break iscsiadm output parsing. [1] https://github.com/mikechristie/open-iscsi/commit/181af9a [2] https://bugzilla.redhat.com/show_bug.cgi?id=1067173 --- tests/Makefile.am | 6 +++ tests/viriscsitest.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 tests/viriscsitest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 90f70ff..a1a89ab 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -141,6 +141,7 @@ test_programs = virshtest sockettest \ virendiantest \ virfiletest \ viridentitytest \ + viriscsitest \ virkeycodetest \ virlockspacetest \ virlogtest \ @@ -643,6 +644,7 @@ storagevolxml2argvtest_SOURCES = \ testutils.c testutils.h storagevolxml2argvtest_LDADD = \ ../src/libvirt_driver_storage_impl.la $(LDADDS) + else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c endif ! WITH_STORAGE @@ -787,6 +789,10 @@ viridentitytest_DEPENDENCIES = libsecurityselinuxhelper.la \ ../src/libvirt.la endif WITH_SELINUX +viriscsitest_SOURCES = \ + viriscsitest.c testutils.h testutils.c +viriscsitest_LDADD = $(LDADDS) + virkeycodetest_SOURCES = \ virkeycodetest.c testutils.h testutils.c virkeycodetest_LDADD = $(LDADDS) diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c new file mode 100644 index 0000000..0b09834 --- /dev/null +++ b/tests/viriscsitest.c @@ -0,0 +1,137 @@ +/* + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Jan Tomko <jtomko@redhat.com> + */ + +#include <config.h> + +#ifdef WIN32 +int +main(void) +{ + return EXIT_AM_SKIP; +} +#else +# define __VIR_COMMAND_PRIV_H_ALLOW__ + +# include "testutils.h" +# include "vircommandpriv.h" +# include "viriscsi.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +static const char *iscsiadmSessionOutput = +"tcp: [1] 10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test\n" +"tcp: [2] 10.20.30.41:3260,1 iqn.2005-05.example:example1:iscsi.hello\n" +"tcp: [3] 10.20.30.42:3260,1 iqn.2006-04.example:example1:iscsi.world\n" +"tcp: [5] 10.20.30.43:3260,1 iqn.2007-04.example:example1:iscsi.foo\n" +"tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" +"tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven\n"; + +static const char *iscsiadmSessionOutputNonFlash = +"tcp: [1] 10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test (non-flash)\n" +"tcp: [2] 10.20.30.41:3260,1 iqn.2005-05.example:example1:iscsi.hello (non-flash)\n" +"tcp: [3] 10.20.30.42:3260,1 iqn.2006-04.example:example1:iscsi.world (non-flash)\n" +"tcp: [5] 10.20.30.43:3260,1 iqn.2007-04.example:example1:iscsi.foo (non-flash)\n" +"tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash)\n" +"tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash)\n"; + +struct testSessionInfo { + const char *device_path; + int output_version; + const char *expected_session; +}; + +static void testIscsiadmCb(const char *const*args, + const char *const*env ATTRIBUTE_UNUSED, + const char *input ATTRIBUTE_UNUSED, + char **output, + char **error ATTRIBUTE_UNUSED, + int *status, + void *opaque) +{ + int *output_version = opaque; + if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "session") && + args[3] == NULL) { + if (*output_version == 1) + ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash)); + else + ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput)); + } else { + *status = -1; + } +} + +static int +testISCSIGetSession(const void *data) +{ + const struct testSessionInfo *info = data; + int ver = info->output_version; + char *actual_session = NULL; + int ret = -1; + + virCommandSetDryRun(NULL, testIscsiadmCb, &ver); + + actual_session = virISCSIGetSession(info->device_path, true); + + if (STRNEQ_NULLABLE(actual_session, info->expected_session)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected session: '%s' got: '%s'", + NULLSTR(info->expected_session), + NULLSTR(actual_session)); + goto cleanup; + } + + ret = 0; + +cleanup: + virCommandSetDryRun(NULL, NULL, NULL); + VIR_FREE(actual_session); + return ret; +} + +static int +mymain(void) +{ + int rv = 0; + +# define DO_SESSION_TEST(name, session) \ + do { \ + struct testSessionInfo info = {name, 0, session}; \ + if (virtTestRun("ISCSI get session test" name, \ + testISCSIGetSession, &info) < 0) \ + rv = -1; \ + info.output_version = 1; \ + if (virtTestRun("ISCSI get (non-flash) session test" name, \ + testISCSIGetSession, &info) < 0) \ + rv = -1; \ + } while (0) + + DO_SESSION_TEST("iqn.2004-06.example:example1:iscsi.test", "1"); + DO_SESSION_TEST("iqn.2009-04.example:example1:iscsi.seven", "7"); + DO_SESSION_TEST("iqn.2009-04.example:example1:iscsi.eight", NULL); + + if (rv < 0) + return EXIT_FAILURE; + return EXIT_SUCCESS; +} + +VIRT_TEST_MAIN(mymain) +#endif /* WIN32 */ -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:32PM +0100, Ján Tomko wrote:
Parse iscsiadm output with and without the recently introduced flashnode info. [1]
This should check that commits like 57e17a7 (fixing [2]) do not break iscsiadm output parsing.
[1] https://github.com/mikechristie/open-iscsi/commit/181af9a [2] https://bugzilla.redhat.com/show_bug.cgi?id=1067173 --- tests/Makefile.am | 6 +++ tests/viriscsitest.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 tests/viriscsitest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index 90f70ff..a1a89ab 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -141,6 +141,7 @@ test_programs = virshtest sockettest \ virendiantest \ virfiletest \ viridentitytest \ + viriscsitest \ virkeycodetest \ virlockspacetest \ virlogtest \ @@ -643,6 +644,7 @@ storagevolxml2argvtest_SOURCES = \ testutils.c testutils.h storagevolxml2argvtest_LDADD = \ ../src/libvirt_driver_storage_impl.la $(LDADDS) + else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c endif ! WITH_STORAGE @@ -787,6 +789,10 @@ viridentitytest_DEPENDENCIES = libsecurityselinuxhelper.la \ ../src/libvirt.la endif WITH_SELINUX
+viriscsitest_SOURCES = \ + viriscsitest.c testutils.h testutils.c +viriscsitest_LDADD = $(LDADDS) + virkeycodetest_SOURCES = \ virkeycodetest.c testutils.h testutils.c virkeycodetest_LDADD = $(LDADDS) diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c new file mode 100644 index 0000000..0b09834 --- /dev/null +++ b/tests/viriscsitest.c @@ -0,0 +1,137 @@ +/* + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Jan Tomko <jtomko@redhat.com> + */ + +#include <config.h> + +#ifdef WIN32 +int +main(void) +{ + return EXIT_AM_SKIP; +} +#else +# define __VIR_COMMAND_PRIV_H_ALLOW__ + +# include "testutils.h" +# include "vircommandpriv.h" +# include "viriscsi.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +static const char *iscsiadmSessionOutput = +"tcp: [1] 10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test\n" +"tcp: [2] 10.20.30.41:3260,1 iqn.2005-05.example:example1:iscsi.hello\n" +"tcp: [3] 10.20.30.42:3260,1 iqn.2006-04.example:example1:iscsi.world\n" +"tcp: [5] 10.20.30.43:3260,1 iqn.2007-04.example:example1:iscsi.foo\n" +"tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" +"tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven\n"; + +static const char *iscsiadmSessionOutputNonFlash = +"tcp: [1] 10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test (non-flash)\n" +"tcp: [2] 10.20.30.41:3260,1 iqn.2005-05.example:example1:iscsi.hello (non-flash)\n" +"tcp: [3] 10.20.30.42:3260,1 iqn.2006-04.example:example1:iscsi.world (non-flash)\n" +"tcp: [5] 10.20.30.43:3260,1 iqn.2007-04.example:example1:iscsi.foo (non-flash)\n" +"tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash)\n" +"tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash)\n";
I've got a minor preference for the strings to be indented relative to the variable declaration ACK with the whitespace change. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

--- tests/viriscsitest.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 0b09834..16a40c7 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -51,6 +51,14 @@ static const char *iscsiadmSessionOutputNonFlash = "tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash)\n" "tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash)\n"; +const char *iscsiadmSendtargetsOutput = +"10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test\n" +"10.20.30.40:3260,1 iqn.2005-05.example:example1:iscsi.hello\n" +"10.20.30.40:3260,1 iqn.2006-04.example:example1:iscsi.world\n" +"10.20.30.40:3260,1 iqn.2007-04.example:example1:iscsi.foo\n" +"10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" +"10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n"; + struct testSessionInfo { const char *device_path; int output_version; @@ -74,6 +82,15 @@ static void testIscsiadmCb(const char *const*args, ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash)); else ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + args[4] && STREQ(args[4], "sendtargets") && + args[5] && STREQ(args[5], "--portal") && + args[6] && STREQ(args[6], "10.20.30.40:3260,1") && + args[7] == NULL) { + ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput)); } else { *status = -1; } @@ -107,6 +124,54 @@ cleanup: return ret; } +struct testScanTargetsInfo { + const char *fake_cmd_output; + const char *portal; + const char **expected_targets; + size_t nexpected; +}; + +static int +testISCSIScanTargets(const void *data) +{ + const struct testScanTargetsInfo *info = data; + size_t ntargets = 0; + char **targets = NULL; + int ret = -1; + size_t i; + + virCommandSetDryRun(NULL, testIscsiadmCb, NULL); + + if (virISCSIScanTargets(info->portal, NULL, + &ntargets, &targets) < 0) + goto cleanup; + + if (info->nexpected != ntargets) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected %zu targets, got %zu", + info->nexpected, ntargets); + goto cleanup; + } + + for (i = 0; i < ntargets; i++) { + if (STRNEQ(info->expected_targets[i], targets[i])) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "Expected target '%s', got '%s'", + info->expected_targets[i], targets[i]); + goto cleanup; + } + } + + ret = 0; + +cleanup: + virCommandSetDryRun(NULL, NULL, NULL); + for (i = 0; i < ntargets; i++) + VIR_FREE(targets[i]); + VIR_FREE(targets); + return ret; +} + static int mymain(void) { @@ -128,6 +193,23 @@ mymain(void) DO_SESSION_TEST("iqn.2009-04.example:example1:iscsi.seven", "7"); DO_SESSION_TEST("iqn.2009-04.example:example1:iscsi.eight", NULL); + const char *targets[] = { + "iqn.2004-06.example:example1:iscsi.test", + "iqn.2005-05.example:example1:iscsi.hello", + "iqn.2006-04.example:example1:iscsi.world", + "iqn.2007-04.example:example1:iscsi.foo", + "iqn.2008-04.example:example1:iscsi.bar", + "iqn.2009-04.example:example1:iscsi.seven" + }; + struct testScanTargetsInfo infoTargets = { + .fake_cmd_output = "iscsiadm_sendtargets", + .portal = "10.20.30.40:3260,1", + .expected_targets = targets, + .nexpected = ARRAY_CARDINALITY(targets), + }; + if (virtTestRun("ISCSI scan targets", testISCSIScanTargets, &infoTargets) < 0) + rv = -1; + if (rv < 0) return EXIT_FAILURE; return EXIT_SUCCESS; -- 1.8.3.2

On Wed, Mar 19, 2014 at 04:52:33PM +0100, Ján Tomko wrote:
--- tests/viriscsitest.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 0b09834..16a40c7 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -51,6 +51,14 @@ static const char *iscsiadmSessionOutputNonFlash = "tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash)\n" "tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash)\n";
+const char *iscsiadmSendtargetsOutput = +"10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test\n" +"10.20.30.40:3260,1 iqn.2005-05.example:example1:iscsi.hello\n" +"10.20.30.40:3260,1 iqn.2006-04.example:example1:iscsi.world\n" +"10.20.30.40:3260,1 iqn.2007-04.example:example1:iscsi.foo\n" +"10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" +"10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
Same note about indenting the strings relative to the variable dcl.
+ struct testSessionInfo { const char *device_path; int output_version; @@ -74,6 +82,15 @@ static void testIscsiadmCb(const char *const*args, ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash)); else ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + args[4] && STREQ(args[4], "sendtargets") && + args[5] && STREQ(args[5], "--portal") && + args[6] && STREQ(args[6], "10.20.30.40:3260,1") && + args[7] == NULL) {
I find myself doing a similar with to compare args. I wonder if we should create a virStringListIsEqual() helper function for comparing a char ** against a statically declared args list. ACK because such a refactoring can be separately done any time. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 03/20/2014 04:01 PM, Daniel P. Berrange wrote:
On Wed, Mar 19, 2014 at 04:52:33PM +0100, Ján Tomko wrote:
--- tests/viriscsitest.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 0b09834..16a40c7 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -51,6 +51,14 @@ static const char *iscsiadmSessionOutputNonFlash = "tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash)\n" "tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash)\n";
+const char *iscsiadmSendtargetsOutput = +"10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test\n" +"10.20.30.40:3260,1 iqn.2005-05.example:example1:iscsi.hello\n" +"10.20.30.40:3260,1 iqn.2006-04.example:example1:iscsi.world\n" +"10.20.30.40:3260,1 iqn.2007-04.example:example1:iscsi.foo\n" +"10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" +"10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
Same note about indenting the strings relative to the variable dcl.
+ struct testSessionInfo { const char *device_path; int output_version; @@ -74,6 +82,15 @@ static void testIscsiadmCb(const char *const*args, ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash)); else ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + args[4] && STREQ(args[4], "sendtargets") && + args[5] && STREQ(args[5], "--portal") && + args[6] && STREQ(args[6], "10.20.30.40:3260,1") && + args[7] == NULL) {
I find myself doing a similar with to compare args. I wonder if we should create a virStringListIsEqual() helper function for comparing a char ** against a statically declared args list.
ACK because such a refactoring can be separately done any time.
I've added ATTRIBUTEs to function declarations in viriscsi.h, indented the const strings and pushed the series, thank you for the reviews! Jan

Ján Tomko wrote:
On 03/20/2014 04:01 PM, Daniel P. Berrange wrote:
On Wed, Mar 19, 2014 at 04:52:33PM +0100, Ján Tomko wrote:
--- tests/viriscsitest.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+)
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c index 0b09834..16a40c7 100644 --- a/tests/viriscsitest.c +++ b/tests/viriscsitest.c @@ -51,6 +51,14 @@ static const char *iscsiadmSessionOutputNonFlash = "tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash)\n" "tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash)\n";
+const char *iscsiadmSendtargetsOutput = +"10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test\n" +"10.20.30.40:3260,1 iqn.2005-05.example:example1:iscsi.hello\n" +"10.20.30.40:3260,1 iqn.2006-04.example:example1:iscsi.world\n" +"10.20.30.40:3260,1 iqn.2007-04.example:example1:iscsi.foo\n" +"10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n" +"10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
Same note about indenting the strings relative to the variable dcl.
+ struct testSessionInfo { const char *device_path; int output_version; @@ -74,6 +82,15 @@ static void testIscsiadmCb(const char *const*args, ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutputNonFlash)); else ignore_value(VIR_STRDUP(*output, iscsiadmSessionOutput)); + } else if (args[0] && STREQ(args[0], ISCSIADM) && + args[1] && STREQ(args[1], "--mode") && + args[2] && STREQ(args[2], "discovery") && + args[3] && STREQ(args[3], "--type") && + args[4] && STREQ(args[4], "sendtargets") && + args[5] && STREQ(args[5], "--portal") && + args[6] && STREQ(args[6], "10.20.30.40:3260,1") && + args[7] == NULL) {
I find myself doing a similar with to compare args. I wonder if we should create a virStringListIsEqual() helper function for comparing a char ** against a statically declared args list.
ACK because such a refactoring can be separately done any time.
I've added ATTRIBUTEs to function declarations in viriscsi.h, indented the const strings and pushed the series, thank you for the reviews!
This breaks the build without iscsi backend: gmake[3]: Entering directory `/usr/home/novel/code/libvirt/src' CC util/libvirt_util_la-viriscsi.lo util/viriscsi.c:83:46: error: use of undeclared identifier 'ISCSIADM' virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); ^ util/viriscsi.c:119:46: error: use of undeclared identifier 'ISCSIADM' virCommandPtr cmd = virCommandNewArgList(ISCSIADM, and other similar errors because of ISCSIADM is not defined. Should these files go into STORAGE_DRIVER_ISCSI_SOURCES or UTIL_SOURCES needs to be adjusted depending if we want iscsi backend or not? Roman Bogorodskiy

On 03/21/2014 08:04 AM, Roman Bogorodskiy wrote:
This breaks the build without iscsi backend:
gmake[3]: Entering directory `/usr/home/novel/code/libvirt/src' CC util/libvirt_util_la-viriscsi.lo util/viriscsi.c:83:46: error: use of undeclared identifier 'ISCSIADM' virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); ^ util/viriscsi.c:119:46: error: use of undeclared identifier 'ISCSIADM' virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
and other similar errors because of ISCSIADM is not defined.
I've pushed a fix that always defines ISCSIADM: https://www.redhat.com/archives/libvir-list/2014-March/msg01353.html
Should these files go into STORAGE_DRIVER_ISCSI_SOURCES or UTIL_SOURCES needs to be adjusted depending if we want iscsi backend or not?
I think it's useful to test them even without the iscsi storage backend. Jan
participants (3)
-
Daniel P. Berrange
-
Ján Tomko
-
Roman Bogorodskiy