It quacks like a bool. --- src/storage/storage_backend_iscsi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ada6c48..531d044 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -92,7 +92,7 @@ virStorageBackendISCSIExtractSession(virStoragePoolObjPtr pool, static char * virStorageBackendISCSISession(virStoragePoolObjPtr pool, - int probe) + bool probe) { /* * # iscsiadm --mode session @@ -646,7 +646,7 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } - if ((session = virStorageBackendISCSISession(pool, 1)) != NULL) { + if ((session = virStorageBackendISCSISession(pool, true)) != NULL) { *isActive = true; VIR_FREE(session); } @@ -806,7 +806,7 @@ virStorageBackendISCSIStartPool(virConnectPtr conn, return -1; } - if ((session = virStorageBackendISCSISession(pool, 1)) == NULL) { + if ((session = virStorageBackendISCSISession(pool, true)) == NULL) { if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) goto cleanup; /* @@ -843,7 +843,7 @@ virStorageBackendISCSIRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, pool->def->allocation = pool->def->capacity = pool->def->available = 0; - if ((session = virStorageBackendISCSISession(pool, 0)) == NULL) + if ((session = virStorageBackendISCSISession(pool, false)) == NULL) goto cleanup; if (virStorageBackendISCSIRescanLUNs(pool, session) < 0) goto cleanup; -- 1.8.3.2

Open-coding one VIR_FREE in the test suite just doesn't seem right. --- src/conf/storage_conf.c | 13 +++++++++---- src/conf/storage_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ac323d0..9c2962f 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -357,6 +357,13 @@ virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter) } void +virStoragePoolSourceDeviceClear(virStoragePoolSourceDevicePtr dev) +{ + VIR_FREE(dev->freeExtents); + VIR_FREE(dev->path); +} + +void virStoragePoolSourceClear(virStoragePoolSourcePtr source) { size_t i; @@ -369,10 +376,8 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) } VIR_FREE(source->hosts); - for (i = 0; i < source->ndevice; i++) { - VIR_FREE(source->devices[i].freeExtents); - VIR_FREE(source->devices[i].path); - } + for (i = 0; i < source->ndevice; i++) + virStoragePoolSourceDeviceClear(&source->devices[i]); VIR_FREE(source->devices); VIR_FREE(source->dir); VIR_FREE(source->name); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 251b968..636c9aa 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -432,6 +432,7 @@ int virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool); void virStorageVolDefFree(virStorageVolDefPtr def); void virStoragePoolSourceClear(virStoragePoolSourcePtr source); +void virStoragePoolSourceDeviceClear(virStoragePoolSourceDevicePtr dev); void virStoragePoolSourceFree(virStoragePoolSourcePtr source); void virStoragePoolDefFree(virStoragePoolDefPtr def); void virStoragePoolObjFree(virStoragePoolObjPtr pool); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f1607cd..ea16cf5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -710,6 +710,7 @@ virStoragePoolObjUnlock; virStoragePoolSourceAdapterTypeTypeFromString; virStoragePoolSourceAdapterTypeTypeToString; virStoragePoolSourceClear; +virStoragePoolSourceDeviceClear; virStoragePoolSourceFindDuplicate; virStoragePoolSourceFindDuplicateDevices; virStoragePoolSourceFree; -- 1.8.3.2

On Wed, Mar 12, 2014 at 02:08:14PM +0100, Ján Tomko wrote: Also add in #ifndef __VIR_COMMAND_PRIV_H_ALLOW__ # error "vircommandpriv.h may only be included by vircommand.c or test suites" #endif and then make vircommand.c and any relevant test suites #define this symbol before including the file. ACK with that 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 :|

After this file is set, all commands executed will be replaced by cat <file>. This is useful for testing functions that parse output of asynchronous commands. --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 33 +++++++++++++++++++++++++++++++++ src/util/vircommandpriv.h | 1 + 3 files changed, 35 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ea16cf5..8c15519 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1123,6 +1123,7 @@ virCommandSetInputFD; virCommandSetMaxFiles; virCommandSetMaxMemLock; virCommandSetMaxProcesses; +virCommandSetMockOutputFile; virCommandSetOutputBuffer; virCommandSetOutputFD; virCommandSetPidFile; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 7a799f2..f954141 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -133,6 +133,9 @@ struct _virCommand { /* See virCommandSetDryRun for description for this variable */ static virBufferPtr dryRunBuffer; +/* See virCommandSetMockOutputFile */ +static const char *mockOutputFile; + /* * virCommandFDIsSet: * @fd: FD to test @@ -2278,6 +2281,19 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) goto cleanup; } + if (mockOutputFile) { + VIR_DEBUG("Replacing %s with cat '%s'", str, mockOutputFile); + for (i = 0; i < cmd->nargs; i++) + VIR_FREE(cmd->args[i]); + cmd->nargs = 0; + + virCommandAddArgList(cmd, "cat", mockOutputFile, NULL); + if (cmd->has_error) { + virReportOOMError(); + goto cleanup; + } + } + VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); ret = virExec(cmd); VIR_DEBUG("Command result %d, with PID %d", @@ -2729,3 +2745,20 @@ virCommandSetDryRun(virBufferPtr buf) { dryRunBuffer = buf; } + +/** + * virCommandSetMockOutputFile: + * @path path to the file that should be concatenated + * instead of running a command + * + * After calling this function, every command run will be replaced + * by 'cat @path'. This is useful for testing functions that + * parse output of asynchronous commands. + * + * NULL argument restores normal behavior. + */ +void +virCommandSetMockOutputFile(const char *path) +{ + mockOutputFile = path; +} diff --git a/src/util/vircommandpriv.h b/src/util/vircommandpriv.h index 2fbf3de..4d83f00 100644 --- a/src/util/vircommandpriv.h +++ b/src/util/vircommandpriv.h @@ -25,4 +25,5 @@ # include "vircommand.h" void virCommandSetDryRun(virBufferPtr buf); +void virCommandSetMockOutputFile(const char *path); #endif /* __VIR_COMMAND_PRIV_H__ */ -- 1.8.3.2

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 --- src/Makefile.am | 3 +- src/storage/storage_backend_iscsi.c | 4 +- src/storage/storage_backend_iscsipriv.h | 31 ++++++ tests/Makefile.am | 14 ++- tests/storageiscsidata/iscsiadm_session | 6 ++ tests/storageiscsidata/iscsiadm_session_nonflash | 6 ++ tests/storageiscsitest.c | 115 +++++++++++++++++++++++ 7 files changed, 174 insertions(+), 5 deletions(-) create mode 100644 src/storage/storage_backend_iscsipriv.h create mode 100644 tests/storageiscsidata/iscsiadm_session create mode 100644 tests/storageiscsidata/iscsiadm_session_nonflash create mode 100644 tests/storageiscsitest.c diff --git a/src/Makefile.am b/src/Makefile.am index 25b33a7..055af83 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -823,7 +823,8 @@ STORAGE_DRIVER_LVM_SOURCES = \ storage/storage_backend_logical.c STORAGE_DRIVER_ISCSI_SOURCES = \ - storage/storage_backend_iscsi.h storage/storage_backend_iscsi.c + storage/storage_backend_iscsi.h storage/storage_backend_iscsi.c \ + storage/storage_backend_iscsipriv.h STORAGE_DRIVER_SCSI_SOURCES = \ storage/storage_backend_scsi.h storage/storage_backend_scsi.c diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 531d044..143412d 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -36,7 +36,7 @@ #include "driver.h" #include "virerror.h" #include "storage_backend_scsi.h" -#include "storage_backend_iscsi.h" +#include "storage_backend_iscsipriv.h" #include "viralloc.h" #include "virlog.h" #include "virfile.h" @@ -90,7 +90,7 @@ virStorageBackendISCSIExtractSession(virStoragePoolObjPtr pool, return 0; } -static char * +char * virStorageBackendISCSISession(virStoragePoolObjPtr pool, bool probe) { diff --git a/src/storage/storage_backend_iscsipriv.h b/src/storage/storage_backend_iscsipriv.h new file mode 100644 index 0000000..25a2509 --- /dev/null +++ b/src/storage/storage_backend_iscsipriv.h @@ -0,0 +1,31 @@ +/* + * storage_backend_iscsipriv.h: functions for thesting the iscsi backend + * + * 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_STORAGE_BACKEND_ISCSIPRIV_H__ +# define __VIR_STORAGE_BACKEND_ISCSIPRIV_H__ + +# include "storage_backend_iscsi.h" + +char * +virStorageBackendISCSISession(virStoragePoolObjPtr pool, + bool probe); + + +#endif /* __VIR_STORAGE_BACKEND_ISCSIPRIV_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 3267ad3..b4147ed 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -105,6 +105,7 @@ EXTRA_DIST = \ storagepoolschematest \ storagepoolxml2xmlin \ storagepoolxml2xmlout \ + storageiscsidata \ storagevolxml2argvdata \ storagevolschemadata \ storagevolschematest \ @@ -247,7 +248,8 @@ endif WITH_STORAGE_SHEEPDOG test_programs += nwfilterxml2xmltest if WITH_STORAGE -test_programs += storagevolxml2argvtest +test_programs += storagevolxml2argvtest \ + storageiscsitest endif WITH_STORAGE if WITH_LINUX @@ -642,8 +644,16 @@ storagevolxml2argvtest_SOURCES = \ testutils.c testutils.h storagevolxml2argvtest_LDADD = \ ../src/libvirt_driver_storage_impl.la $(LDADDS) + +storageiscsitest_SOURCES = \ + storageiscsitest.c \ + testutils.c testutils.h +storageiscsitest_LDADD = \ + ../src/libvirt_driver_storage_impl.la $(LDADDS) + else ! WITH_STORAGE -EXTRA_DIST += storagevolxml2argvtest.c +EXTRA_DIST += storagevolxml2argvtest.c \ + storageiscsitest.c endif ! WITH_STORAGE storagevolxml2xmltest_SOURCES = \ diff --git a/tests/storageiscsidata/iscsiadm_session b/tests/storageiscsidata/iscsiadm_session new file mode 100644 index 0000000..52425be --- /dev/null +++ b/tests/storageiscsidata/iscsiadm_session @@ -0,0 +1,6 @@ +tcp: [1] 10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test (non-flash) +tcp: [2] 10.20.30.41:3260,1 iqn.2005-05.example:example1:iscsi.hello (non-flash) +tcp: [3] 10.20.30.42:3260,1 iqn.2006-04.example:example1:iscsi.world (non-flash) +tcp: [5] 10.20.30.43:3260,1 iqn.2007-04.example:example1:iscsi.foo (non-flash) +tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash) +tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash) diff --git a/tests/storageiscsidata/iscsiadm_session_nonflash b/tests/storageiscsidata/iscsiadm_session_nonflash new file mode 100644 index 0000000..52425be --- /dev/null +++ b/tests/storageiscsidata/iscsiadm_session_nonflash @@ -0,0 +1,6 @@ +tcp: [1] 10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test (non-flash) +tcp: [2] 10.20.30.41:3260,1 iqn.2005-05.example:example1:iscsi.hello (non-flash) +tcp: [3] 10.20.30.42:3260,1 iqn.2006-04.example:example1:iscsi.world (non-flash) +tcp: [5] 10.20.30.43:3260,1 iqn.2007-04.example:example1:iscsi.foo (non-flash) +tcp: [6] 10.20.30.44:3260,1 iqn.2008-04.example:example1:iscsi.bar (non-flash) +tcp: [7] 10.20.30.45:3260,1 iqn.2009-04.example:example1:iscsi.seven (non-flash) diff --git a/tests/storageiscsitest.c b/tests/storageiscsitest.c new file mode 100644 index 0000000..212f524 --- /dev/null +++ b/tests/storageiscsitest.c @@ -0,0 +1,115 @@ +/* + * 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(a)redhat.com&gt; + */ + +#include <config.h> +#include <stdlib.h> + +#include "testutils.h" +#include "storage_conf.h" +#include "storage/storage_backend_iscsipriv.h" +#include "vircommandpriv.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct testSessionInfo { + const char *device_path; + const char *fake_cmd_output; + const char *expected_session; +}; + +static int +testISCSISession(const void *data) +{ + const struct testSessionInfo *info = data; + char *cmdout = NULL; + char *tmp = NULL; + int ret = -1; + char *actual_session = NULL; + + /* virStorageBackendISCSISession requires virStoragePoolObjPtr, + * but it only uses obj.def->source.devices[0].path */ + virStoragePoolObj obj = { .def = NULL }; + virStoragePoolSourceDevice srcdev = { .path = NULL }; + + if (VIR_ALLOC(obj.def) < 0) + goto cleanup; + + if (VIR_STRDUP(srcdev.path, info->device_path) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(obj.def->source.devices, + obj.def->source.ndevice, + srcdev) < 0) + goto cleanup; + + if (virAsprintf(&cmdout, "%s/storageiscsidata/%s", abs_srcdir, + info->fake_cmd_output) < 0) + goto cleanup; + + virCommandSetMockOutputFile(cmdout); + + actual_session = virStorageBackendISCSISession(&obj, 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)); + } else { + ret = 0; + } + +cleanup: + virCommandSetMockOutputFile(NULL); + virStoragePoolSourceDeviceClear(&srcdev); + virStoragePoolDefFree(obj.def); + VIR_FREE(cmdout); + VIR_FREE(tmp); + VIR_FREE(actual_session); + return ret; +} + +static int +mymain(void) +{ + int rv = 0; + +#define DO_SESSION_TEST(name, session) \ + do { \ + struct testSessionInfo info = {name, "iscsiadm_session", session}; \ + if (virtTestRun("ISCSI session test" name, \ + testISCSISession, &info) < 0) \ + rv = -1; \ + info.fake_cmd_output = "iscsiadm_session_nonflash"; \ + if (virtTestRun("ISCSI (non-flash) session test" name, \ + testISCSISession, &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) -- 1.8.3.2

On Wed, Mar 12, 2014 at 02:08:17PM +0100, Ján Tomko wrote: If we use the callback approach I suggested, then this data can just be a constant string inlined in the storageiscsitest.c file. I think that is preferrable since we're not likely to need to have 100's of external data files as we do for other tests 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 :|