[libvirt] [PATCH 0/7] Test parsing of iscsiadm output
Introduce virCommandSetMockOutputFile for faking output of asynchronous commands and use it to test parsing of iscsiadm output in virStorageBackendISCSISession and virStorageBackendISCSIScanTargets Ján Tomko (7): Change virStorageBackendISCSISession 'probe' arg to bool Introduce virStoragePoolSourceDeviceClear Use size_t for ndevice in pool source definition Introduce vircommandpriv.h for functions used by tests Add virCommandSetMockOutputFile Add test for virStorageBackendISCSISession Add a test for virStorageBackendISCSIScanTargets src/Makefile.am | 5 +- src/conf/storage_conf.c | 36 ++--- src/conf/storage_conf.h | 3 +- src/libvirt_private.syms | 2 + src/storage/storage_backend_iscsi.c | 14 +- src/storage/storage_backend_iscsipriv.h | 36 +++++ src/util/vircommand.c | 35 ++++- src/util/vircommand.h | 2 - src/util/vircommandpriv.h | 29 ++++ tests/Makefile.am | 14 +- tests/storageiscsidata/iscsiadm_sendtargets | 6 + tests/storageiscsidata/iscsiadm_session | 6 + tests/storageiscsidata/iscsiadm_session_nonflash | 6 + tests/storageiscsitest.c | 186 +++++++++++++++++++++++ tests/virkmodtest.c | 2 +- tests/virnetdevbandwidthtest.c | 2 +- 16 files changed, 350 insertions(+), 34 deletions(-) create mode 100644 src/storage/storage_backend_iscsipriv.h create mode 100644 src/util/vircommandpriv.h create mode 100644 tests/storageiscsidata/iscsiadm_sendtargets create mode 100644 tests/storageiscsidata/iscsiadm_session create mode 100644 tests/storageiscsidata/iscsiadm_session_nonflash create mode 100644 tests/storageiscsitest.c -- 1.8.3.2
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
On Wed, Mar 12, 2014 at 02:08:11PM +0100, Ján Tomko wrote:
It quacks like a bool. --- src/storage/storage_backend_iscsi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 :|
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
This allows it to be used by the VIR_*_ELEMENT macros. Also use them for parsing the definiton and remove the redundant freeing of 'nodeset' before jumping to the cleanup label. --- src/conf/storage_conf.c | 23 ++++++++++------------- src/conf/storage_conf.h | 2 +- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9c2962f..39fb416 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -642,23 +642,20 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if (nsource < 0) goto cleanup; - if (nsource > 0) { - if (VIR_ALLOC_N(source->devices, nsource) < 0) { - VIR_FREE(nodeset); + for (i = 0; i < nsource; i++) { + virStoragePoolSourceDevice dev = { .path = NULL }; + dev.path = virXMLPropString(nodeset[i], "path"); + + if (dev.path == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool source device path")); goto cleanup; } - for (i = 0; i < nsource; i++) { - char *path = virXMLPropString(nodeset[i], "path"); - if (path == NULL) { - VIR_FREE(nodeset); - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing storage pool source device path")); - goto cleanup; - } - source->devices[i].path = path; + if (VIR_APPEND_ELEMENT(source->devices, source->ndevice, dev) < 0) { + virStoragePoolSourceDeviceClear(&dev); + goto cleanup; } - source->ndevice = nsource; } source->dir = virXPathString("string(./dir/@path)", ctxt); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 636c9aa..e410f41 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -266,7 +266,7 @@ struct _virStoragePoolSource { virStoragePoolSourceHostPtr hosts; /* And either one or more devices ... */ - int ndevice; + size_t ndevice; virStoragePoolSourceDevicePtr devices; /* Or a directory */ -- 1.8.3.2
On Wed, Mar 12, 2014 at 02:08:13PM +0100, Ján Tomko wrote:
This allows it to be used by the VIR_*_ELEMENT macros.
Also use them for parsing the definiton and remove the redundant freeing of 'nodeset' before jumping to the cleanup label. --- src/conf/storage_conf.c | 23 ++++++++++------------- src/conf/storage_conf.h | 2 +- 2 files changed, 11 insertions(+), 14 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 :|
So far it's just virCommandSetDryRun. --- src/Makefile.am | 2 +- src/util/vircommand.c | 2 +- src/util/vircommand.h | 2 -- src/util/vircommandpriv.h | 28 ++++++++++++++++++++++++++++ tests/virkmodtest.c | 2 +- tests/virnetdevbandwidthtest.c | 2 +- 6 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 src/util/vircommandpriv.h diff --git a/src/Makefile.am b/src/Makefile.am index a88b258..25b33a7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -91,7 +91,7 @@ UTIL_SOURCES = \ util/virbuffer.c util/virbuffer.h \ util/vircgroup.c util/vircgroup.h util/vircgrouppriv.h \ util/virclosecallbacks.c util/virclosecallbacks.h \ - util/vircommand.c util/vircommand.h \ + util/vircommand.c util/vircommand.h util/vircommandpriv.h \ util/virconf.c util/virconf.h \ util/vircrypto.c util/vircrypto.h \ util/virdbus.c util/virdbus.h util/virdbuspriv.h \ diff --git a/src/util/vircommand.c b/src/util/vircommand.c index db4166f..7a799f2 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -40,7 +40,7 @@ # include <sys/apparmor.h> #endif -#include "vircommand.h" +#include "vircommandpriv.h" #include "viralloc.h" #include "virerror.h" #include "virutil.h" diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 7485edc..10b4fa2 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -186,6 +186,4 @@ void virCommandAbort(virCommandPtr cmd); void virCommandFree(virCommandPtr cmd); void virCommandDoAsyncIO(virCommandPtr cmd); - -void virCommandSetDryRun(virBufferPtr buf); #endif /* __VIR_COMMAND_H__ */ diff --git a/src/util/vircommandpriv.h b/src/util/vircommandpriv.h new file mode 100644 index 0000000..2fbf3de --- /dev/null +++ b/src/util/vircommandpriv.h @@ -0,0 +1,28 @@ +/* + * vircommandpriv.h: Functions for testing virCommand APIs + * + * 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_COMMAND_PRIV_H__ +# define __VIR_COMMAND_PRIV_H__ + +# include "vircommand.h" + +void virCommandSetDryRun(virBufferPtr buf); +#endif /* __VIR_COMMAND_PRIV_H__ */ diff --git a/tests/virkmodtest.c b/tests/virkmodtest.c index c6f5a72..e23ef67 100644 --- a/tests/virkmodtest.c +++ b/tests/virkmodtest.c @@ -23,7 +23,7 @@ #ifdef __linux__ # include <stdlib.h> -# include "vircommand.h" +# include "vircommandpriv.h" # include "virkmod.h" # include "virstring.h" diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index 073fdf8..ab5d312 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -21,7 +21,7 @@ #include <config.h> #include "testutils.h" -#include "vircommand.h" +#include "vircommandpriv.h" #include "virnetdevbandwidth.h" #include "netdev_bandwidth_conf.c" -- 1.8.3.2
On Wed, Mar 12, 2014 at 02:08:14PM +0100, Ján Tomko wrote:
So far it's just virCommandSetDryRun. --- src/Makefile.am | 2 +- src/util/vircommand.c | 2 +- src/util/vircommand.h | 2 -- src/util/vircommandpriv.h | 28 ++++++++++++++++++++++++++++ tests/virkmodtest.c | 2 +- tests/virnetdevbandwidthtest.c | 2 +- 6 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 src/util/vircommandpriv.h 1 diff --git a/src/Makefile.am b/src/Makefile.am index a88b258..25b33a7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -91,7 +91,7 @@ UTIL_SOURCES = \ util/virbuffer.c util/virbuffer.h \ util/vircgroup.c util/vircgroup.h util/vircgrouppriv.h \ util/virclosecallbacks.c util/virclosecallbacks.h \ - util/vircommand.c util/vircommand.h \ + util/vircommand.c util/vircommand.h util/vircommandpriv.h \ util/virconf.c util/virconf.h \ util/vircrypto.c util/vircrypto.h \ util/virdbus.c util/virdbus.h util/virdbuspriv.h \ diff --git a/src/util/vircommand.c b/src/util/vircommand.c index db4166f..7a799f2 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -40,7 +40,7 @@ # include <sys/apparmor.h> #endif
-#include "vircommand.h" +#include "vircommandpriv.h" #include "viralloc.h" #include "virerror.h" #include "virutil.h" diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 7485edc..10b4fa2 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -186,6 +186,4 @@ void virCommandAbort(virCommandPtr cmd); void virCommandFree(virCommandPtr cmd);
void virCommandDoAsyncIO(virCommandPtr cmd); - -void virCommandSetDryRun(virBufferPtr buf); #endif /* __VIR_COMMAND_H__ */ diff --git a/src/util/vircommandpriv.h b/src/util/vircommandpriv.h new file mode 100644 index 0000000..2fbf3de --- /dev/null +++ b/src/util/vircommandpriv.h @@ -0,0 +1,28 @@ +/* + * vircommandpriv.h: Functions for testing virCommand APIs + * + * 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/>. + * + */ +
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.
+#ifndef __VIR_COMMAND_PRIV_H__ +# define __VIR_COMMAND_PRIV_H__ + +# include "vircommand.h" + +void virCommandSetDryRun(virBufferPtr buf); +#endif /* __VIR_COMMAND_PRIV_H__ */
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 :|
On 03/12/2014 03:24 PM, Daniel P. Berrange wrote:
On Wed, Mar 12, 2014 at 02:08:14PM +0100, Ján Tomko wrote:
So far it's just virCommandSetDryRun. --- src/Makefile.am | 2 +- src/util/vircommand.c | 2 +- src/util/vircommand.h | 2 -- src/util/vircommandpriv.h | 28 ++++++++++++++++++++++++++++ tests/virkmodtest.c | 2 +- tests/virnetdevbandwidthtest.c | 2 +- 6 files changed, 32 insertions(+), 6 deletions(-) create mode 100644 src/util/vircommandpriv.h
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.
+#ifndef __VIR_COMMAND_PRIV_H__ +# define __VIR_COMMAND_PRIV_H__ + +# include "vircommand.h" + +void virCommandSetDryRun(virBufferPtr buf); +#endif /* __VIR_COMMAND_PRIV_H__ */
ACK with that change.
I've changed that and pushed patches 1-4. Thanks for the review. Jan
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
On Wed, Mar 12, 2014 at 02:08:15PM +0100, Ján Tomko wrote:
After this file is set, all commands executed will be replaced by cat <file>.
I think this is a bit too special case and unecessarily forces use of an external cat command. I think your test would be easier if using this callback approach I've just proposed https://www.redhat.com/archives/libvir-list/2014-March/msg00733.html 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/12/2014 03:26 PM, Daniel P. Berrange wrote:
On Wed, Mar 12, 2014 at 02:08:15PM +0100, Ján Tomko wrote:
After this file is set, all commands executed will be replaced by cat <file>. I think this is a bit too special case and unecessarily forces use of an external cat command. I think your test would be easier if using this callback approach I've just proposed
https://www.redhat.com/archives/libvir-list/2014-March/msg00733.html
Definitely nicer, but it doesn't work with asynchronous commands. Jan
On Wed, Mar 12, 2014 at 04:33:22PM +0100, Ján Tomko wrote:
On 03/12/2014 03:26 PM, Daniel P. Berrange wrote:
On Wed, Mar 12, 2014 at 02:08:15PM +0100, Ján Tomko wrote:
After this file is set, all commands executed will be replaced by cat <file>. I think this is a bit too special case and unecessarily forces use of an external cat command. I think your test would be easier if using this callback approach I've just proposed
https://www.redhat.com/archives/libvir-list/2014-March/msg00733.html
Definitely nicer, but it doesn't work with asynchronous commands.
Looking at the code it seems that the places which us virCommandRunAsync will create a FILE * from the output file descriptor, and then read full lines of output processing each in turn. What do you think about re-writing that so that it uses virCommandRun, and provides a char **output buffer, which we then just call virStringSplit(output, "\n") instead to get the list of lines to process. 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 --- 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@redhat.com> + */ + +#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
Check the iscsiadm output parsing by reading it from a file. --- src/storage/storage_backend_iscsi.c | 2 +- src/storage/storage_backend_iscsipriv.h | 5 ++ tests/storageiscsidata/iscsiadm_sendtargets | 6 +++ tests/storageiscsitest.c | 71 +++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 tests/storageiscsidata/iscsiadm_sendtargets diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 143412d..316fb1b 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -470,7 +470,7 @@ virStorageBackendISCSITargetAutologin(const char *portal, } -static int +int virStorageBackendISCSIScanTargets(const char *portal, const char *initiatoriqn, size_t *ntargetsret, diff --git a/src/storage/storage_backend_iscsipriv.h b/src/storage/storage_backend_iscsipriv.h index 25a2509..897bfb4 100644 --- a/src/storage/storage_backend_iscsipriv.h +++ b/src/storage/storage_backend_iscsipriv.h @@ -27,5 +27,10 @@ char * virStorageBackendISCSISession(virStoragePoolObjPtr pool, bool probe); +int +virStorageBackendISCSIScanTargets(const char *portal, + const char *initiatoriqn, + size_t *ntargetsret, + char ***targetsret); #endif /* __VIR_STORAGE_BACKEND_ISCSIPRIV_H__ */ diff --git a/tests/storageiscsidata/iscsiadm_sendtargets b/tests/storageiscsidata/iscsiadm_sendtargets new file mode 100644 index 0000000..acea8ee --- /dev/null +++ b/tests/storageiscsidata/iscsiadm_sendtargets @@ -0,0 +1,6 @@ +10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test +10.20.30.40:3260,1 iqn.2005-05.example:example1:iscsi.hello +10.20.30.40:3260,1 iqn.2006-04.example:example1:iscsi.world +10.20.30.40:3260,1 iqn.2007-04.example:example1:iscsi.foo +10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar +10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven diff --git a/tests/storageiscsitest.c b/tests/storageiscsitest.c index 212f524..304bb12 100644 --- a/tests/storageiscsitest.c +++ b/tests/storageiscsitest.c @@ -86,6 +86,60 @@ 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; + char *cmdout = NULL; + int ret = -1; + size_t i; + + if (virAsprintf(&cmdout, "%s/storageiscsidata/%s", abs_srcdir, + info->fake_cmd_output) < 0) + goto cleanup; + + virCommandSetMockOutputFile(cmdout); + + if (virStorageBackendISCSIScanTargets(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: + virCommandSetMockOutputFile(NULL); + for (i = 0; i < ntargets; i++) + VIR_FREE(targets[i]); + VIR_FREE(targets); + VIR_FREE(cmdout); + return ret; +} + static int mymain(void) { @@ -107,6 +161,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.45: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 12, 2014 at 02:08:17PM +0100, Ján Tomko wrote:
diff --git a/tests/storageiscsidata/iscsiadm_sendtargets b/tests/storageiscsidata/iscsiadm_sendtargets new file mode 100644 index 0000000..acea8ee --- /dev/null +++ b/tests/storageiscsidata/iscsiadm_sendtargets @@ -0,0 +1,6 @@ +10.20.30.40:3260,1 iqn.2004-06.example:example1:iscsi.test +10.20.30.40:3260,1 iqn.2005-05.example:example1:iscsi.hello +10.20.30.40:3260,1 iqn.2006-04.example:example1:iscsi.world +10.20.30.40:3260,1 iqn.2007-04.example:example1:iscsi.foo +10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar +10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven
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 :|
On Wed, Mar 12, 2014 at 02:08:10PM +0100, Ján Tomko wrote:
Introduce virCommandSetMockOutputFile for faking output of asynchronous commands and use it to test parsing of iscsiadm output in virStorageBackendISCSISession and virStorageBackendISCSIScanTargets
Ján Tomko (7): Change virStorageBackendISCSISession 'probe' arg to bool Introduce virStoragePoolSourceDeviceClear Use size_t for ndevice in pool source definition Introduce vircommandpriv.h for functions used by tests Add virCommandSetMockOutputFile Add test for virStorageBackendISCSISession Add a test for virStorageBackendISCSIScanTargets
Strangely I've still not received patch 2, but ACK to that patch based on what's in the mailing list archives. If you want to do any more work in this area, I think one other good idea would be to introduce a src/util/viriscsi.{c,h} helper API which exposes a set of APIs for core iSCSI operations we need. This would isolate all the iscsiadm command code in one place, separated from the storage backend implementation, so both sides of the code would be easier to follow. I think I'd extend this API to other stuff the storage backends do too. eg a virlvm.{c,h} and a virscsi.{c,h} etc, to again isolate code that is invoking commands + parsing their output, from the code that actually uses this functionality. 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 :|
participants (2)
-
Daniel P. Berrange -
Ján Tomko