On 06/29/2018 11:01 AM, Michal Privoznik wrote: > When scanning for targets, iSCSI might give different results > depending on the interface used. This is basically just name of assuming this means whether the initiatoriqn interface was used > config file under /etc/iscsi/ifaces to use. The file contains > initiator IQN thus different results claim. > Strange to have activity here - was there a bz? Or something found by the (I assume) GSoC project: https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html Touching something that's been avoided for 8 years is always scary, but if it's broken, then sure it ought to be fixed. > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > src/storage/storage_backend_iscsi.c | 4 +- > src/util/viriscsi.c | 78 ++++++++++++++++++++++++++++++++++--- > src/util/viriscsi.h | 1 + > tests/viriscsitest.c | 2 +- > 4 files changed, 77 insertions(+), 8 deletions(-) > This patch fails to compile for me: In file included from util/viriscsi.c:32:0: util/viriscsi.c: In function 'virISCSIScanTargets': util/virerror.h:187:5: error: this statement may fall through [-Werror=implicit-fallthrough=] virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __FUNCTION__, __LINE__, __VA_ARGS__) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ util/viriscsi.c:479:13: note: in expansion of macro 'virReportError' virReportError(VIR_ERR_OPERATION_FAILED, ^~~~~~~~~~~~~~ util/viriscsi.c:482:9: note: here case IQN_ERROR: ^~~~ > diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c > index 7871d1915b..3b9dddb4fd 100644 > --- a/src/storage/storage_backend_iscsi.c > +++ b/src/storage/storage_backend_iscsi.c > @@ -194,7 +194,9 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec, > if (!(portal = virStorageBackendISCSIPortal(source))) > goto cleanup; > > - if (virISCSIScanTargets(portal, &ntargets, &targets) < 0) > + if (virISCSIScanTargets(portal, > + source->initiator.iqn, NIT: This could go on the previous line > + &ntargets, &targets) < 0) > goto cleanup; > > if (VIR_ALLOC_N(list.sources, ntargets) < 0) > diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c > index 44788056fd..365669aac2 100644 > --- a/src/util/viriscsi.c > +++ b/src/util/viriscsi.c > @@ -40,6 +40,13 @@ > VIR_LOG_INIT("util.iscsi"); > > > +static int > +virISCSIScanTargetsInternal(const char *portal, > + const char *ifacename, > + size_t *ntargetsret, > + char ***targetsret); > + > + > struct virISCSISessionData { > char *session; > const char *devpath; > @@ -286,9 +293,10 @@ virISCSIConnection(const char *portal, > * iscsiadm doesn't let you send commands to the Interface IQN, > * unless you've first issued a 'sendtargets' command to the > * portal. Without the sendtargets all that is received is a > - * "iscsiadm: No records found" > + * "iscsiadm: No records found". However, we must ensure that > + * the command is issued over interface name we invented above. "invented" - again is this the make sure it's issued over the initiatoriqn interface? > */ > - if (virISCSIScanTargets(portal, NULL, NULL) < 0) > + if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 0) > goto cleanup; > > break; > @@ -371,10 +379,11 @@ virISCSIGetTargets(char **const groups, > } > > > -int > -virISCSIScanTargets(const char *portal, > - size_t *ntargetsret, > - char ***targetsret) > +static int > +virISCSIScanTargetsInternal(const char *portal, > + const char *ifacename, > + size_t *ntargetsret, > + char ***targetsret) > { > /** > * > @@ -400,6 +409,12 @@ virISCSIScanTargets(const char *portal, > "--op", "nonpersistent", > NULL); > > + if (ifacename) { > + virCommandAddArgList(cmd, > + "--interface", ifacename, > + NULL); > + } > + Could just be one line to avoid the { } > memset(&list, 0, sizeof(list)); > > if (virCommandRunRegex(cmd, > @@ -425,6 +440,57 @@ virISCSIScanTargets(const char *portal, > return ret; > } > > + > +/** > + * virISCSIScanTargets: > + * @portal: iSCSI portal > + * @initiatoriqn: Initiator IQN > + * @ntargets: number of items in @targetsret array > + * @targets: array of targets > + * > + * For given @portal issue sendtargets command. Optionally, > + * @initiatoriqn can be set to override default configuration. > + * The targets are stored into @targets array and the size of > + * the array is stored into @ntargets. > + * > + * Returns: 0 on success, > + * -1 otherwise (with error reported) > + */ > +int > +virISCSIScanTargets(const char *portal, > + const char *initiatoriqn, > + size_t *ntargets, > + char ***targets) > +{ > + char *ifacename = NULL; > + int ret = -1; > + > + if (ntargets) > + *ntargets = 0; > + if (targets) > + *targets = NULL; > + > + if (initiatoriqn) { > + switch ((virStorageBackendIQNFound(initiatoriqn, &ifacename))) { > + case IQN_FOUND: > + break; > + > + case IQN_MISSING: > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("no iSCSI interface defined for IQN %s"), > + initiatoriqn); ATTRIBUTE_FALLTHROUGH; With a couple of adjustments and some more explanation why we're here - as in what is failing in the current scheme, Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> John > + case IQN_ERROR: > + default: > + return -1; > + } > + } > + > + ret = virISCSIScanTargetsInternal(portal, ifacename, ntargets, targets); > + VIR_FREE(ifacename); > + return ret; > +} > + > + > /* > * virISCSINodeNew: > * @portal: address for iSCSI target > diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h > index a44beeaf67..31b589dbf9 100644 > --- a/src/util/viriscsi.h > +++ b/src/util/viriscsi.h > @@ -49,6 +49,7 @@ virISCSIRescanLUNs(const char *session) > > int > virISCSIScanTargets(const char *portal, > + const char *initiatoriqn, > size_t *ntargetsret, > char ***targetsret) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c > index aa4ba57707..4bdb782e36 100644 > --- a/tests/viriscsitest.c > +++ b/tests/viriscsitest.c > @@ -145,7 +145,7 @@ testISCSIScanTargets(const void *data) > > virCommandSetDryRun(NULL, testIscsiadmCb, NULL); > > - if (virISCSIScanTargets(info->portal, &ntargets, &targets) < 0) > + if (virISCSIScanTargets(info->portal, NULL, &ntargets, &targets) < 0) > goto cleanup; > > if (info->nexpected != ntargets) { > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
