10:01 a.m.
I've noticed that I'm unable to start my iscsi pool if I put my
own IQN into the pool XML. This is very surprising because
iscsiadm would read it from initiatorname.conf file, right? Well,
no. If IQN is provided in pool XML the code calls slightly
different iscsiadm commands (e.g. to set up iSCSI interface) and
we don't pass the interface everywhere we should.
Michal Privoznik (5):
virStorageBackendIQNFound: Fix ret value assignment
virStorageBackendIQNFound: Rename out label
virStorageBackendIQNFound: Rework iscsiadm output parsing
virISCSIScanTargets: Honour iSCSI interface
virISCSIScanTargets: Allow making targets persistent
src/storage/storage_backend_iscsi.c | 5 +-
src/util/viriscsi.c | 197 +++++++++++++++++++++++-------------
src/util/viriscsi.h | 2 +
tests/viriscsitest.c | 3 +-
4 files changed, 137 insertions(+), 70 deletions(-)
--
2.16.4
10:01 a.m.
New subject: [libvirt] [PATCH 1/5] virStorageBackendIQNFound: Fix ret value assignment
The return value of this function is overwritten more times than
I am comfortable with.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index d4c745a1af..a308ee4bac 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -117,15 +117,16 @@ static int
virStorageBackendIQNFound(const char *initiatoriqn,
char **ifacename)
{
- int ret = IQN_MISSING, fd = -1;
+ int ret = IQN_ERROR, fd = -1;
char ebuf[64];
FILE *fp = NULL;
char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface",
NULL);
+ *ifacename = 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);
@@ -135,24 +136,20 @@ virStorageBackendIQNFound(const char *initiatoriqn,
memset(line, 0, LINE_SIZE);
virCommandSetOutputFD(cmd, &fd);
- if (virCommandRunAsync(cmd, NULL) < 0) {
- ret = IQN_ERROR;
+ if (virCommandRunAsync(cmd, NULL) < 0)
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'"),
@@ -169,27 +166,27 @@ virStorageBackendIQNFound(const char *initiatoriqn,
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;
+
+ if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
goto out;
- }
+
VIR_DEBUG("Found interface '%s' with IQN '%s'",
*ifacename, iqn);
- ret = IQN_FOUND;
break;
}
}
if (virCommandWait(cmd, NULL) < 0)
- ret = IQN_ERROR;
+ goto out;
+
+ ret = *ifacename ? IQN_FOUND : IQN_MISSING;
out:
- if (ret == IQN_MISSING)
+ if (ret != IQN_FOUND)
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
VIR_FREE(line);
--
2.16.4
5:29 p.m.
New subject: [libvirt] [PATCH 1/5] virStorageBackendIQNFound: Fix ret value assignment
On 06/29/2018 11:01 AM, Michal Privoznik wrote:
The return value of this function is overwritten more times than
I am comfortable with.
True, but let's actually say what you're doing...
Perform some method clean-up to follow more accepted coding standards:
* Initialize @ret to error value and prove otherwise.
* Initialize *ifacename to NULL
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
Touching Dave's 8+ year old code (commit id 6aabcb5), lucky you. I
believe this "feature" was never documented too - now that your name is
on top you can own it :-P!
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index d4c745a1af..a308ee4bac 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -117,15 +117,16 @@ static int
virStorageBackendIQNFound(const char *initiatoriqn,
char **ifacename)
{
- int ret = IQN_MISSING, fd = -1;
+ int ret = IQN_ERROR, fd = -1;
char ebuf[64];
FILE *fp = NULL;
char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface",
NULL);
+ *ifacename = 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);
@@ -135,24 +136,20 @@ virStorageBackendIQNFound(const char *initiatoriqn,
memset(line, 0, LINE_SIZE);
virCommandSetOutputFD(cmd, &fd);
- if (virCommandRunAsync(cmd, NULL) < 0) {
- ret = IQN_ERROR;
+ if (virCommandRunAsync(cmd, NULL) < 0)
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'"),
@@ -169,27 +166,27 @@ virStorageBackendIQNFound(const char *initiatoriqn,
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;
+
+ if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
goto out;
- }
+
VIR_DEBUG("Found interface '%s' with IQN '%s'",
*ifacename, iqn);
- ret = IQN_FOUND;
break;
}
}
if (virCommandWait(cmd, NULL) < 0)
- ret = IQN_ERROR;
+ goto out;
+
+ ret = *ifacename ? IQN_FOUND : IQN_MISSING;
out:
While you're at it s/out/cleanup
- if (ret == IQN_MISSING)
+ if (ret != IQN_FOUND)
IDC since it's VIR_DEBUG, but you'll get this message on IQN_ERROR too
as well as the virReportError... I would think that's fairly obvious on
the error paths
With some minor cleanups...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
VIR_DEBUG("Could not find interface with IQN
'%s'", iqn);
VIR_FREE(line);
10:01 a.m.
New subject: [libvirt] [PATCH 2/5] virStorageBackendIQNFound: Rename out label
This is in fact 'cleanup' label and it should be named as such.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index a308ee4bac..2e55b3c10b 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -130,21 +130,21 @@ virStorageBackendIQNFound(const char *initiatoriqn,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not allocate memory for output of
'%s'"),
ISCSIADM);
- goto out;
+ goto cleanup;
}
memset(line, 0, LINE_SIZE);
virCommandSetOutputFD(cmd, &fd);
if (virCommandRunAsync(cmd, NULL) < 0)
- goto out;
+ goto cleanup;
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)));
- goto out;
+ goto cleanup;
}
while (fgets(line, LINE_SIZE, fp) != NULL) {
@@ -154,7 +154,7 @@ virStorageBackendIQNFound(const char *initiatoriqn,
_("Unexpected line > %d characters "
"when parsing output of '%s'"),
LINE_SIZE, ISCSIADM);
- goto out;
+ goto cleanup;
}
*newline = '\0';
@@ -169,11 +169,11 @@ virStorageBackendIQNFound(const char *initiatoriqn,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing space when parsing output "
"of '%s'"), ISCSIADM);
- goto out;
+ goto cleanup;
}
if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
- goto out;
+ goto cleanup;
VIR_DEBUG("Found interface '%s' with IQN '%s'",
*ifacename, iqn);
break;
@@ -181,11 +181,11 @@ virStorageBackendIQNFound(const char *initiatoriqn,
}
if (virCommandWait(cmd, NULL) < 0)
- goto out;
+ goto cleanup;
ret = *ifacename ? IQN_FOUND : IQN_MISSING;
- out:
+ cleanup:
if (ret != IQN_FOUND)
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
--
2.16.4
5:30 p.m.
New subject: [libvirt] [PATCH 2/5] virStorageBackendIQNFound: Rename out label
On 06/29/2018 11:01 AM, Michal Privoznik wrote:
This is in fact 'cleanup' label and it should be named as
such.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10:01 a.m.
New subject: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing
Firstly, we can utilize virCommandSetOutputBuffer() API which
will collect the command output for us. Secondly, sscanf()-ing
through each line is easier to understand (and more robust) than
jumping over a string with strchr().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
1 file changed, 34 insertions(+), 51 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 2e55b3c10b..44788056fd 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
-#define LINE_SIZE 4096
#define IQN_FOUND 1
#define IQN_MISSING 0
#define IQN_ERROR -1
@@ -117,71 +116,56 @@ static int
virStorageBackendIQNFound(const char *initiatoriqn,
char **ifacename)
{
- int ret = IQN_ERROR, fd = -1;
- char ebuf[64];
- FILE *fp = NULL;
- char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
+ int ret = IQN_ERROR;
+ char *outbuf = NULL;
+ char *line = NULL;
+ char *iface = NULL;
+ char *iqn = NULL;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface",
NULL);
*ifacename = NULL;
- if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not allocate memory for output of
'%s'"),
- ISCSIADM);
+ virCommandSetOutputBuffer(cmd, &outbuf);
+ if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
- }
- memset(line, 0, LINE_SIZE);
+ /* Example of data we are dealing with:
+ * default tcp,<empty>,<empty>,<empty>,<empty>
+ * iser iser,<empty>,<empty>,<empty>,<empty>
+ * libvirt-iface-253db048
tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
+ */
- virCommandSetOutputFD(cmd, &fd);
- if (virCommandRunAsync(cmd, NULL) < 0)
- goto cleanup;
+ line = outbuf;
+ while (line) {
+ char *newline;
+ int num;
- 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)));
- goto cleanup;
- }
+ if (!(newline = strchr(line, '\n')))
+ break;
- while (fgets(line, LINE_SIZE, fp) != NULL) {
- newline = strrchr(line, '\n');
- if (newline == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected line > %d characters "
- "when parsing output of '%s'"),
- LINE_SIZE, ISCSIADM);
- goto cleanup;
- }
*newline = '\0';
- iqn = strrchr(line, ',');
- if (iqn == NULL)
- continue;
- iqn++;
+ VIR_FREE(iface);
+ VIR_FREE(iqn);
+ num = sscanf(line, "%ms %*[^,],%*[^,],%*[^,],%*[^,],%ms", &iface,
&iqn);
+
+ if (num != 2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed output of %s: %s"),
+ ISCSIADM, line);
+ goto cleanup;
+ }
if (STREQ(iqn, initiatoriqn)) {
- token = strchr(line, ' ');
- if (!token) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing space when parsing output "
- "of '%s'"), ISCSIADM);
- goto cleanup;
- }
-
- if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
- goto cleanup;
+ VIR_STEAL_PTR(*ifacename, iface);
VIR_DEBUG("Found interface '%s' with IQN '%s'",
*ifacename, iqn);
break;
}
- }
- if (virCommandWait(cmd, NULL) < 0)
- goto cleanup;
+ line = newline + 1;
+ }
ret = *ifacename ? IQN_FOUND : IQN_MISSING;
@@ -189,11 +173,10 @@ virStorageBackendIQNFound(const char *initiatoriqn,
if (ret != IQN_FOUND)
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
- VIR_FREE(line);
- VIR_FORCE_FCLOSE(fp);
- VIR_FORCE_CLOSE(fd);
+ VIR_FREE(iqn);
+ VIR_FREE(iface);
+ VIR_FREE(outbuf);
virCommandFree(cmd);
-
return ret;
}
--
2.16.4
2:39 a.m.
New subject: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing
On Fri, Jun 29, 2018 at 17:01:49 +0200, Michal Privoznik wrote:
Firstly, we can utilize virCommandSetOutputBuffer() API which
will collect the command output for us. Secondly, sscanf()-ing
through each line is easier to understand (and more robust) than
jumping over a string with strchr().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
1 file changed, 34 insertions(+), 51 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 2e55b3c10b..44788056fd 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
[...]
@@ -117,71 +116,56 @@ static int
virStorageBackendIQNFound(const char *initiatoriqn,
char **ifacename)
{
- int ret = IQN_ERROR, fd = -1;
- char ebuf[64];
- FILE *fp = NULL;
- char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
+ int ret = IQN_ERROR;
+ char *outbuf = NULL;
+ char *line = NULL;
+ char *iface = NULL;
+ char *iqn = NULL;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface",
NULL);
*ifacename = NULL;
- if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not allocate memory for output of
'%s'"),
- ISCSIADM);
+ virCommandSetOutputBuffer(cmd, &outbuf);
+ if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
- }
- memset(line, 0, LINE_SIZE);
+ /* Example of data we are dealing with:
+ * default tcp,<empty>,<empty>,<empty>,<empty>
+ * iser iser,<empty>,<empty>,<empty>,<empty>
+ * libvirt-iface-253db048
tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
+ */
- virCommandSetOutputFD(cmd, &fd);
- if (virCommandRunAsync(cmd, NULL) < 0)
- goto cleanup;
+ line = outbuf;
+ while (line) {
This is severely misleading and makes this loop technically infinite. Or
just checks that output buffer was not null. Both operations should be
made explicit. Or you meant *line or line[0]
+ char *newline;
+ int num;
- 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)));
- goto cleanup;
- }
+ if (!(newline = strchr(line, '\n')))
+ break;
This is ther reason why it's working at this point.
- while (fgets(line, LINE_SIZE, fp) != NULL) {
- newline = strrchr(line, '\n');
9:26 a.m.
New subject: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing
On 07/02/2018 09:39 AM, Peter Krempa wrote:
On Fri, Jun 29, 2018 at 17:01:49 +0200, Michal Privoznik wrote:
> Firstly, we can utilize virCommandSetOutputBuffer() API which
> will collect the command output for us. Secondly, sscanf()-ing
> through each line is easier to understand (and more robust) than
> jumping over a string with strchr().
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
> 1 file changed, 34 insertions(+), 51 deletions(-)
>
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 2e55b3c10b..44788056fd 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
[...]
> @@ -117,71 +116,56 @@ static int
> virStorageBackendIQNFound(const char *initiatoriqn,
> char **ifacename)
> {
> - int ret = IQN_ERROR, fd = -1;
> - char ebuf[64];
> - FILE *fp = NULL;
> - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
> + int ret = IQN_ERROR;
> + char *outbuf = NULL;
> + char *line = NULL;
> + char *iface = NULL;
> + char *iqn = NULL;
> virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
> "--mode", "iface",
NULL);
>
> *ifacename = NULL;
>
> - if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not allocate memory for output of
'%s'"),
> - ISCSIADM);
> + virCommandSetOutputBuffer(cmd, &outbuf);
> + if (virCommandRun(cmd, NULL) < 0)
> goto cleanup;
> - }
>
> - memset(line, 0, LINE_SIZE);
> + /* Example of data we are dealing with:
> + * default tcp,<empty>,<empty>,<empty>,<empty>
> + * iser iser,<empty>,<empty>,<empty>,<empty>
> + * libvirt-iface-253db048
tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
> + */
>
> - virCommandSetOutputFD(cmd, &fd);
> - if (virCommandRunAsync(cmd, NULL) < 0)
> - goto cleanup;
> + line = outbuf;
> + while (line) {
This is severely misleading and makes this loop technically infinite. Or
just checks that output buffer was not null. Both operations should be
made explicit. Or you meant *line or line[0]
Okay, how about:
while (line && *line) {
Michal
6:18 p.m.
New subject: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing
On 06/29/2018 11:01 AM, Michal Privoznik wrote:
Firstly, we can utilize virCommandSetOutputBuffer() API which
will collect the command output for us. Secondly, sscanf()-ing
through each line is easier to understand (and more robust) than
jumping over a string with strchr().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
1 file changed, 34 insertions(+), 51 deletions(-)
I assume virCommandSetOutputBuffer didn't exist when this code was created.
Also, why do I have this recollection related to portability of sscanf?
I know we use it elsewhere, but some oddball issue that someone like
Eric may recollect or know about.
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 2e55b3c10b..44788056fd 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
-#define LINE_SIZE 4096
#define IQN_FOUND 1
#define IQN_MISSING 0
#define IQN_ERROR -1
@@ -117,71 +116,56 @@ static int
virStorageBackendIQNFound(const char *initiatoriqn,
char **ifacename)
{
- int ret = IQN_ERROR, fd = -1;
- char ebuf[64];
- FILE *fp = NULL;
- char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
+ int ret = IQN_ERROR;
+ char *outbuf = NULL;
+ char *line = NULL;
+ char *iface = NULL;
+ char *iqn = NULL;
virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
"--mode", "iface",
NULL);
*ifacename = NULL;
- if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not allocate memory for output of
'%s'"),
- ISCSIADM);
+ virCommandSetOutputBuffer(cmd, &outbuf);
+ if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
- }
- memset(line, 0, LINE_SIZE);
+ /* Example of data we are dealing with:
+ * default tcp,<empty>,<empty>,<empty>,<empty>
+ * iser iser,<empty>,<empty>,<empty>,<empty>
+ * libvirt-iface-253db048
tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
+ */
Nice to have an example especially since this is a bit of a edge case...
Another option - virStringSplitCount on the "\n" to get the number of
lines and then on each line again using "," to tear apart the pieces.
This I assume works as I don't have a initiatoriqn set up to test.
Any thoughts/recollections about sscanf? I assume it'll be felt that
virStringSplit might be overkill, but at least it's for other purposes
and perhaps less susceptible to the line && *line adjustment.
IDC - let's see if sscanf causes any other recollections before R-by'ing.
John
- virCommandSetOutputFD(cmd, &fd);
- if (virCommandRunAsync(cmd, NULL) < 0)
- goto cleanup;
+ line = outbuf;
+ while (line) {
+ char *newline;
+ int num;
- 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)));
- goto cleanup;
- }
+ if (!(newline = strchr(line, '\n')))
+ break;
- while (fgets(line, LINE_SIZE, fp) != NULL) {
- newline = strrchr(line, '\n');
- if (newline == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected line > %d characters "
- "when parsing output of '%s'"),
- LINE_SIZE, ISCSIADM);
- goto cleanup;
- }
*newline = '\0';
- iqn = strrchr(line, ',');
- if (iqn == NULL)
- continue;
- iqn++;
+ VIR_FREE(iface);
+ VIR_FREE(iqn);
+ num = sscanf(line, "%ms %*[^,],%*[^,],%*[^,],%*[^,],%ms", &iface,
&iqn);
+
+ if (num != 2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed output of %s: %s"),
+ ISCSIADM, line);
+ goto cleanup;
+ }
if (STREQ(iqn, initiatoriqn)) {
- token = strchr(line, ' ');
- if (!token) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Missing space when parsing output "
- "of '%s'"), ISCSIADM);
- goto cleanup;
- }
-
- if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
- goto cleanup;
+ VIR_STEAL_PTR(*ifacename, iface);
VIR_DEBUG("Found interface '%s' with IQN '%s'",
*ifacename, iqn);
break;
}
- }
- if (virCommandWait(cmd, NULL) < 0)
- goto cleanup;
+ line = newline + 1;
+ }
ret = *ifacename ? IQN_FOUND : IQN_MISSING;
@@ -189,11 +173,10 @@ virStorageBackendIQNFound(const char *initiatoriqn,
if (ret != IQN_FOUND)
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
- VIR_FREE(line);
- VIR_FORCE_FCLOSE(fp);
- VIR_FORCE_CLOSE(fd);
+ VIR_FREE(iqn);
+ VIR_FREE(iface);
+ VIR_FREE(outbuf);
virCommandFree(cmd);
-
return ret;
}
12:08 a.m.
New subject: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing
On 07/03/2018 01:18 AM, John Ferlan wrote:
On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> Firstly, we can utilize virCommandSetOutputBuffer() API which
> will collect the command output for us. Secondly, sscanf()-ing
> through each line is easier to understand (and more robust) than
> jumping over a string with strchr().
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
> 1 file changed, 34 insertions(+), 51 deletions(-)
>
I assume virCommandSetOutputBuffer didn't exist when this code was created.
Perhaps. Let me check git log. .. .. And yes, you are right.
virCommandSetOutputBuffer was introduced among with other basic
virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
function was introduced by Dave in Jan 2010 so he couldn't have used it.
Also, why do I have this recollection related to portability of sscanf?
I know we use it elsewhere, but some oddball issue that someone like
Eric may recollect or know about.
I don't know about anything. But since we use it in our code pretty
freely I assumed there is no problem with it.
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 2e55b3c10b..44788056fd 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>
>
>
> -#define LINE_SIZE 4096
> #define IQN_FOUND 1
> #define IQN_MISSING 0
> #define IQN_ERROR -1
> @@ -117,71 +116,56 @@ static int
> virStorageBackendIQNFound(const char *initiatoriqn,
> char **ifacename)
> {
> - int ret = IQN_ERROR, fd = -1;
> - char ebuf[64];
> - FILE *fp = NULL;
> - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
> + int ret = IQN_ERROR;
> + char *outbuf = NULL;
> + char *line = NULL;
> + char *iface = NULL;
> + char *iqn = NULL;
> virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
> "--mode", "iface",
NULL);
>
> *ifacename = NULL;
>
> - if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Could not allocate memory for output of
'%s'"),
> - ISCSIADM);
> + virCommandSetOutputBuffer(cmd, &outbuf);
> + if (virCommandRun(cmd, NULL) < 0)
> goto cleanup;
> - }
>
> - memset(line, 0, LINE_SIZE);
> + /* Example of data we are dealing with:
> + * default tcp,<empty>,<empty>,<empty>,<empty>
> + * iser iser,<empty>,<empty>,<empty>,<empty>
> + * libvirt-iface-253db048
tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
> + */
Nice to have an example especially since this is a bit of a edge case...
Another option - virStringSplitCount on the "\n" to get the number of
lines and then on each line again using "," to tear apart the pieces.
This I assume works as I don't have a initiatoriqn set up to test.
Any thoughts/recollections about sscanf? I assume it'll be felt that
virStringSplit might be overkill,
Indeed.
but at least it's for other purposes
and perhaps less susceptible to the line && *line adjustment.
I like sscanf() more because it's fewer lines of code, the variables I
need are assigned value immediately and also memory footprint is smaller
(I guess) since there's no need to store multiple arrays.
Michal
7:51 a.m.
New subject: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing
On 07/03/2018 01:08 AM, Michal Prívozník wrote:
On 07/03/2018 01:18 AM, John Ferlan wrote:
>
>
> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>> will collect the command output for us. Secondly, sscanf()-ing
>> through each line is easier to understand (and more robust) than
>> jumping over a string with strchr().
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/util/viriscsi.c | 85 +++++++++++++++++++++--------------------------------
>> 1 file changed, 34 insertions(+), 51 deletions(-)
>>
>
> I assume virCommandSetOutputBuffer didn't exist when this code was created.
Perhaps. Let me check git log. .. .. And yes, you are right.
virCommandSetOutputBuffer was introduced among with other basic
virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
function was introduced by Dave in Jan 2010 so he couldn't have used it.
>
> Also, why do I have this recollection related to portability of sscanf?
> I know we use it elsewhere, but some oddball issue that someone like
> Eric may recollect or know about.
I don't know about anything. But since we use it in our code pretty
freely I assumed there is no problem with it.
>
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 2e55b3c10b..44788056fd 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
>> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>>
>>
>>
>> -#define LINE_SIZE 4096
>> #define IQN_FOUND 1
>> #define IQN_MISSING 0
>> #define IQN_ERROR -1
>> @@ -117,71 +116,56 @@ static int
>> virStorageBackendIQNFound(const char *initiatoriqn,
>> char **ifacename)
>> {
>> - int ret = IQN_ERROR, fd = -1;
>> - char ebuf[64];
>> - FILE *fp = NULL;
>> - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>> + int ret = IQN_ERROR;
>> + char *outbuf = NULL;
>> + char *line = NULL;
>> + char *iface = NULL;
>> + char *iqn = NULL;
>> virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>> "--mode",
"iface", NULL);
>>
>> *ifacename = NULL;
>>
>> - if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Could not allocate memory for output of
'%s'"),
>> - ISCSIADM);
>> + virCommandSetOutputBuffer(cmd, &outbuf);
>> + if (virCommandRun(cmd, NULL) < 0)
>> goto cleanup;
>> - }
>>
>> - memset(line, 0, LINE_SIZE);
>> + /* Example of data we are dealing with:
>> + * default tcp,<empty>,<empty>,<empty>,<empty>
>> + * iser iser,<empty>,<empty>,<empty>,<empty>
>> + * libvirt-iface-253db048
tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
>> + */
>
> Nice to have an example especially since this is a bit of a edge case...
>
> Another option - virStringSplitCount on the "\n" to get the number of
> lines and then on each line again using "," to tear apart the pieces.
> This I assume works as I don't have a initiatoriqn set up to test.
>
> Any thoughts/recollections about sscanf? I assume it'll be felt that
> virStringSplit might be overkill,
Indeed.
> but at least it's for other purposes
> and perhaps less susceptible to the line && *line adjustment.
I like sscanf() more because it's fewer lines of code, the variables I
need are assigned value immediately and also memory footprint is smaller
(I guess) since there's no need to store multiple arrays.
Understood - it's probably something really early on when I first
started w/ libvirt and using sscanf was discouraged for something that
has stuck in my head. Another part for me is readability - similar to
the various [i]scsi code that uses regex's in order to parse output.
That format is just a mish-mash of search patterns that causes my eyes
to complain.
Perhaps it's best to see this along with the combined 4&5 "again".
Also
add a non initiatoriqn to the mix (even the comment above) just so show
the various formats.
Hopefully using libiscsi makes all the personally displeasing regex and
sscanf searching code disappear.
John
8:08 a.m.
New subject: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing
On 07/03/2018 02:51 PM, John Ferlan wrote:
On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:18 AM, John Ferlan wrote:
>>
>>
>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>>> will collect the command output for us. Secondly, sscanf()-ing
>>> through each line is easier to understand (and more robust) than
>>> jumping over a string with strchr().
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> src/util/viriscsi.c | 85
+++++++++++++++++++++--------------------------------
>>> 1 file changed, 34 insertions(+), 51 deletions(-)
>>>
>>
>> I assume virCommandSetOutputBuffer didn't exist when this code was created.
>
> Perhaps. Let me check git log. .. .. And yes, you are right.
> virCommandSetOutputBuffer was introduced among with other basic
> virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
> function was introduced by Dave in Jan 2010 so he couldn't have used it.
>
>>
>> Also, why do I have this recollection related to portability of sscanf?
>> I know we use it elsewhere, but some oddball issue that someone like
>> Eric may recollect or know about.
>
> I don't know about anything. But since we use it in our code pretty
> freely I assumed there is no problem with it.
>
>>
>>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>>> index 2e55b3c10b..44788056fd 100644
>>> --- a/src/util/viriscsi.c
>>> +++ b/src/util/viriscsi.c
>>> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>>>
>>>
>>>
>>> -#define LINE_SIZE 4096
>>> #define IQN_FOUND 1
>>> #define IQN_MISSING 0
>>> #define IQN_ERROR -1
>>> @@ -117,71 +116,56 @@ static int
>>> virStorageBackendIQNFound(const char *initiatoriqn,
>>> char **ifacename)
>>> {
>>> - int ret = IQN_ERROR, fd = -1;
>>> - char ebuf[64];
>>> - FILE *fp = NULL;
>>> - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>>> + int ret = IQN_ERROR;
>>> + char *outbuf = NULL;
>>> + char *line = NULL;
>>> + char *iface = NULL;
>>> + char *iqn = NULL;
>>> virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>> "--mode",
"iface", NULL);
>>>
>>> *ifacename = NULL;
>>>
>>> - if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>>> - _("Could not allocate memory for output of
'%s'"),
>>> - ISCSIADM);
>>> + virCommandSetOutputBuffer(cmd, &outbuf);
>>> + if (virCommandRun(cmd, NULL) < 0)
>>> goto cleanup;
>>> - }
>>>
>>> - memset(line, 0, LINE_SIZE);
>>> + /* Example of data we are dealing with:
>>> + * default tcp,<empty>,<empty>,<empty>,<empty>
>>> + * iser iser,<empty>,<empty>,<empty>,<empty>
>>> + * libvirt-iface-253db048
tcp,<empty>,<empty>,<empty>,iqn.2017-03.com.user:client
>>> + */
>>
>> Nice to have an example especially since this is a bit of a edge case...
>>
>> Another option - virStringSplitCount on the "\n" to get the number of
>> lines and then on each line again using "," to tear apart the pieces.
>> This I assume works as I don't have a initiatoriqn set up to test.
>>
>> Any thoughts/recollections about sscanf? I assume it'll be felt that
>> virStringSplit might be overkill,
>
> Indeed.
>
>> but at least it's for other purposes
>> and perhaps less susceptible to the line && *line adjustment.
>
> I like sscanf() more because it's fewer lines of code, the variables I
> need are assigned value immediately and also memory footprint is smaller
> (I guess) since there's no need to store multiple arrays.
>
Understood - it's probably something really early on when I first
started w/ libvirt and using sscanf was discouraged for something that
has stuck in my head. Another part for me is readability - similar to
the various [i]scsi code that uses regex's in order to parse output.
That format is just a mish-mash of search patterns that causes my eyes
to complain.
Yup.
Perhaps it's best to see this along with the combined 4&5 "again".
Also
add a non initiatoriqn to the mix (even the comment above) just so show
the various formats.
Hopefully using libiscsi makes all the personally displeasing regex and
sscanf searching code disappear.
Unfortunately it will not. The libiscsi GSoC project aims on creating
new type of pool "iscsi-direct". It is going to be completely new pool
type (storage driver backend) which will share some similarities with
iscsi pool we have but in some aspects it will be more like rbd pool,
because it is not going to have any <target/> (=no host representation
of LUNs).
Michal
10:01 a.m.
New subject: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface
When scanning for targets, iSCSI might give different results
depending on the interface used. This is basically just name of
config file under /etc/iscsi/ifaces to use. The file contains
initiator IQN thus different results claim.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
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(-)
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,
+ &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.
*/
- 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);
+ }
+
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);
+ 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) {
--
2.16.4
6:38 p.m.
New subject: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface
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(a)redhat.com>
---
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(a)redhat.com>
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) {
12:08 a.m.
New subject: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface
On 07/03/2018 01:38 AM, John Ferlan wrote:
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
Yes.
> 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.
There is no BZ. And yes, the GSoC project got me experimenting (because
for libiscsi backend we will have to make IQN required since libiscsi
does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
while experimenting I've tired to put my own IQN into iscsi pool we
already have and found this bug.
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> 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:
Ah, okay. My gcc doesn't warn me about this. Wonder if I have something
misconfigured.
^~~~
> 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?
Yes. And we construct the interface name at the beginning of this function.
Michal
7:38 a.m.
New subject: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface
On 07/03/2018 01:08 AM, Michal Prívozník wrote:
On 07/03/2018 01:38 AM, John Ferlan wrote:
>
>
> 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
Yes.
>
>> 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.
There is no BZ. And yes, the GSoC project got me experimenting (because
for libiscsi backend we will have to make IQN required since libiscsi
does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
while experimenting I've tired to put my own IQN into iscsi pool we
already have and found this bug.
FWIW: To a degree this is restoring the initiatoriqn argument that
commit 027986f5 removed...
[...]
>> +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?
Yes. And we construct the interface name at the beginning of this function.
Oh right, the virStorageBackendCreateIfaceIQN call which creates that
libvirt-iface-* name. That's just one of those context things that you
know when you've been working through the code ;-).
Still without the next patch, does this work?
John
8:08 a.m.
New subject: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface
On 07/03/2018 02:38 PM, John Ferlan wrote:
On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:38 AM, John Ferlan wrote:
>>
>>
>> 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
>
> Yes.
>
>>
>>> 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.
>
> There is no BZ. And yes, the GSoC project got me experimenting (because
> for libiscsi backend we will have to make IQN required since libiscsi
> does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
> while experimenting I've tired to put my own IQN into iscsi pool we
> already have and found this bug.
>
FWIW: To a degree this is restoring the initiatoriqn argument that
commit 027986f5 removed...
Yes, but I did not think it is worth mentioning anywhere. We change code
all the time, and if we were even to blame which commits caused what ..
it's a endless spiral IMO.
[...]
>>> +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?
>
> Yes. And we construct the interface name at the beginning of this function.
>
Oh right, the virStorageBackendCreateIfaceIQN call which creates that
libvirt-iface-* name. That's just one of those context things that you
know when you've been working through the code ;-).
Still without the next patch, does this work?
Not really because even though targets are refreshed their configs are
not saved and thus login still fails.
Michal
10:01 a.m.
New subject: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent
After new iSCSI interface is successfully set up, we issue
sendtargets command. However, after 56057900dc53df490d we don't
update the host config which in turn makes login fail because
iscsiadm is unable to find any matching record for the interface.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 1 +
src/util/viriscsi.c | 21 ++++++++++++++++++---
src/util/viriscsi.h | 1 +
tests/viriscsitest.c | 3 ++-
4 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 3b9dddb4fd..6242cd0fac 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -196,6 +196,7 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
if (virISCSIScanTargets(portal,
source->initiator.iqn,
+ false,
&ntargets, &targets) < 0)
goto cleanup;
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 365669aac2..baf41c5be1 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -43,6 +43,7 @@ VIR_LOG_INIT("util.iscsi");
static int
virISCSIScanTargetsInternal(const char *portal,
const char *ifacename,
+ bool persist,
size_t *ntargetsret,
char ***targetsret);
@@ -295,8 +296,10 @@ virISCSIConnection(const char *portal,
* portal. Without the sendtargets all that is received is a
* "iscsiadm: No records found". However, we must ensure that
* the command is issued over interface name we invented above.
+ * AND that targets are made persistent.
*/
- if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 0)
+ if (virISCSIScanTargetsInternal(portal, ifacename,
+ true, NULL, NULL) < 0)
goto cleanup;
break;
@@ -382,6 +385,7 @@ virISCSIGetTargets(char **const groups,
static int
virISCSIScanTargetsInternal(const char *portal,
const char *ifacename,
+ bool persist,
size_t *ntargetsret,
char ***targetsret)
{
@@ -406,9 +410,14 @@ virISCSIScanTargetsInternal(const char *portal,
"--mode", "discovery",
"--type",
"sendtargets",
"--portal", portal,
- "--op",
"nonpersistent",
NULL);
+ if (!persist) {
+ virCommandAddArgList(cmd,
+ "--op", "nonpersistent",
+ NULL);
+ }
+
if (ifacename) {
virCommandAddArgList(cmd,
"--interface", ifacename,
@@ -445,6 +454,7 @@ virISCSIScanTargetsInternal(const char *portal,
* virISCSIScanTargets:
* @portal: iSCSI portal
* @initiatoriqn: Initiator IQN
+ * @persists: whether scanned targets should be saved
* @ntargets: number of items in @targetsret array
* @targets: array of targets
*
@@ -453,12 +463,16 @@ virISCSIScanTargetsInternal(const char *portal,
* The targets are stored into @targets array and the size of
* the array is stored into @ntargets.
*
+ * If @persist is true, then targets returned by iSCSI portal are
+ * made persistent on the host (their config is saved).
+ *
* Returns: 0 on success,
* -1 otherwise (with error reported)
*/
int
virISCSIScanTargets(const char *portal,
const char *initiatoriqn,
+ bool persist,
size_t *ntargets,
char ***targets)
{
@@ -485,7 +499,8 @@ virISCSIScanTargets(const char *portal,
}
}
- ret = virISCSIScanTargetsInternal(portal, ifacename, ntargets, targets);
+ ret = virISCSIScanTargetsInternal(portal, ifacename,
+ persist, ntargets, targets);
VIR_FREE(ifacename);
return ret;
}
diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h
index 31b589dbf9..4da9becfb2 100644
--- a/src/util/viriscsi.h
+++ b/src/util/viriscsi.h
@@ -50,6 +50,7 @@ virISCSIRescanLUNs(const char *session)
int
virISCSIScanTargets(const char *portal,
const char *initiatoriqn,
+ bool persist,
size_t *ntargetsret,
char ***targetsret)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
index 4bdb782e36..3bb3993196 100644
--- a/tests/viriscsitest.c
+++ b/tests/viriscsitest.c
@@ -145,7 +145,8 @@ testISCSIScanTargets(const void *data)
virCommandSetDryRun(NULL, testIscsiadmCb, NULL);
- if (virISCSIScanTargets(info->portal, NULL, &ntargets, &targets) < 0)
+ if (virISCSIScanTargets(info->portal, NULL,
+ false, &ntargets, &targets) < 0)
goto cleanup;
if (info->nexpected != ntargets) {
--
2.16.4
6:40 p.m.
New subject: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent
On 06/29/2018 11:01 AM, Michal Privoznik wrote:
After new iSCSI interface is successfully set up, we issue
s/new/a new/
s/issue/issue a/
sendtargets command. However, after 56057900dc53df490d we don't
update the host config which in turn makes login fail because
iscsiadm is unable to find any matching record for the interface.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 1 +
src/util/viriscsi.c | 21 ++++++++++++++++++---
src/util/viriscsi.h | 1 +
tests/viriscsitest.c | 3 ++-
4 files changed, 22 insertions(+), 4 deletions(-)
Like the previous patch - is there a specific bug or something that led
you down this path? Can you show an example of the existing code that's
creating a bad command line and generating an error and then how this
fixes things. It's not like we have tests and for this stuff it's
really nice to have plenty of examples.
diff --git a/src/storage/storage_backend_iscsi.c
b/src/storage/storage_backend_iscsi.c
index 3b9dddb4fd..6242cd0fac 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -196,6 +196,7 @@ virStorageBackendISCSIFindPoolSources(const char *srcSpec,
if (virISCSIScanTargets(portal,
source->initiator.iqn,
+ false,
&ntargets, &targets) < 0)
goto cleanup;
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index 365669aac2..baf41c5be1 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -43,6 +43,7 @@ VIR_LOG_INIT("util.iscsi");
static int
virISCSIScanTargetsInternal(const char *portal,
const char *ifacename,
+ bool persist,
size_t *ntargetsret,
char ***targetsret);
@@ -295,8 +296,10 @@ virISCSIConnection(const char *portal,
* portal. Without the sendtargets all that is received is a
* "iscsiadm: No records found". However, we must ensure that
* the command is issued over interface name we invented above.
s/above./above/
+ * AND that targets are made persistent.
s/AND/and/
Similar to previous - with minor adjustments and explanation,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
*/
- if (virISCSIScanTargetsInternal(portal, ifacename, NULL, NULL) < 0)
+ if (virISCSIScanTargetsInternal(portal, ifacename,
+ true, NULL, NULL) < 0)
goto cleanup;
break;
[...]
12:08 a.m.
New subject: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent
On 07/03/2018 01:40 AM, John Ferlan wrote:
On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> After new iSCSI interface is successfully set up, we issue
s/new/a new/
s/issue/issue a/
> sendtargets command. However, after 56057900dc53df490d we don't
> update the host config which in turn makes login fail because
> iscsiadm is unable to find any matching record for the interface.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/storage/storage_backend_iscsi.c | 1 +
> src/util/viriscsi.c | 21 ++++++++++++++++++---
> src/util/viriscsi.h | 1 +
> tests/viriscsitest.c | 3 ++-
> 4 files changed, 22 insertions(+), 4 deletions(-)
>
Like the previous patch - is there a specific bug or something that led
you down this path? Can you show an example of the existing code that's
creating a bad command line and generating an error and then how this
fixes things. It's not like we have tests and for this stuff it's
really nice to have plenty of examples.
So here is the run without my patches:
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
iscsiadm: No active sessions.
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value
CHAP
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value
$USER
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value
$PASS
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-03316143 --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-03316143 --op update --name iface.initiatorname --value $INITIATOR
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type
sendtargets --portal $PORTAL:3260,1 --op nonpersistent
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143
error : virCommandWait:2600 : internal error: Child process (iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143) unexpected
exit status 21:
iscsiadm: No records found
iscsiadm: No records found
And with my patches:
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
iscsiadm: No active sessions.
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value
CHAP
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value
$USER
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value
$PASS
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-28727243 --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-28727243 --op update --name iface.initiatorname --value $INITIATOR
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type
sendtargets --portal $PORTAL:3260,1 --interface libvirt-iface-28727243
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-28727243
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session -r 1 -R
Thanks,
Michal
7:42 a.m.
New subject: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent
On 07/03/2018 01:08 AM, Michal Prívozník wrote:
On 07/03/2018 01:40 AM, John Ferlan wrote:
>
>
> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>> After new iSCSI interface is successfully set up, we issue
>
> s/new/a new/
> s/issue/issue a/
>
>> sendtargets command. However, after 56057900dc53df490d we don't
>> update the host config which in turn makes login fail because
>> iscsiadm is unable to find any matching record for the interface.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/storage/storage_backend_iscsi.c | 1 +
>> src/util/viriscsi.c | 21 ++++++++++++++++++---
>> src/util/viriscsi.h | 1 +
>> tests/viriscsitest.c | 3 ++-
>> 4 files changed, 22 insertions(+), 4 deletions(-)
>>
>
> Like the previous patch - is there a specific bug or something that led
> you down this path? Can you show an example of the existing code that's
> creating a bad command line and generating an error and then how this
> fixes things. It's not like we have tests and for this stuff it's
> really nice to have plenty of examples.
So here is the run without my patches:
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
iscsiadm: No active sessions.
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value
CHAP
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value
$USER
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value
$PASS
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-03316143 --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-03316143 --op update --name iface.initiatorname --value $INITIATOR
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type
sendtargets --portal $PORTAL:3260,1 --op nonpersistent
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143
error : virCommandWait:2600 : internal error: Child process (iscsiadm --mode node
--portal $PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143)
unexpected exit status 21:
iscsiadm: No records found
iscsiadm: No records found
And with my patches:
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
iscsiadm: No active sessions.
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value
CHAP
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value
$USER
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value
$PASS
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-28727243 --op new
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-28727243 --op update --name iface.initiatorname --value $INITIATOR
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type
sendtargets --portal $PORTAL:3260,1 --interface libvirt-iface-28727243
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-28727243
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session -r 1 -R
So the example helps me understand - thanks! The difference of discovery
being:
fail: iscsiadm --mode discovery --type sendtargets \
--portal $PORTAL:3260,1 \
--op nonpersistent
succ: iscsiadm --mode discovery --type sendtargets \
--portal $PORTAL:3260,1 \
--interface libvirt-iface-28727243
So, what commit 56057900d did to "help" or "fix" auto-login for
sessions
that do not "belong to" libvirt is being called out as causing problems
for the initiatoriqn sessions. Prior to that commit the command was:
iscsiadm --mode discovery --type sendtargets --portal $PORTAL:3260,1
So, I have to wonder "how" or "if" discovery using an initiatoriqn
really worked since there wasn't an --interface argument. Not sure I
have the patience/desire to go through the history of refactors and
adjustments since 6aabcb5 though ;-). Although it does appear that the
original code would make a "node" during it's connection period rather
than doing a sendtargets using the --interface option (and whether that
option existed at the time is a different search through a different set
of code).
Given all of what's been discovered here - I think patch 4 and 5 should
be combined. And rather than adding a new "bool persist" parameter that
only is true when initiatoriqn is passed, let's use the fact that
initiatoriqn was passed in order to:
if (ifacename)
virCommandAddArgList(cmd, "--interface", ifacename, NULL);
else
virCommandAddArgList(cmd, "--op", "nonpersistent", NULL);
Then as long as non initiatoriqn sessions still work, even those without
a libvirt pool associated - we should be able to declare victory. Not
sure "if" non libvirt pool initiatoriqn sessions would be affected by
all this.
John
BTW: Based on that commit, I wonder if you can modify viriscsitest.c a
bit in order to generate the initiatoriqn output as well via a change to
testIscsiadmCb.
8:08 a.m.
New subject: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent
On 07/03/2018 02:42 PM, John Ferlan wrote:
On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:40 AM, John Ferlan wrote:
>>
>>
>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>> After new iSCSI interface is successfully set up, we issue
>>
>> s/new/a new/
>> s/issue/issue a/
>>
>>> sendtargets command. However, after 56057900dc53df490d we don't
>>> update the host config which in turn makes login fail because
>>> iscsiadm is unable to find any matching record for the interface.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> src/storage/storage_backend_iscsi.c | 1 +
>>> src/util/viriscsi.c | 21 ++++++++++++++++++---
>>> src/util/viriscsi.h | 1 +
>>> tests/viriscsitest.c | 3 ++-
>>> 4 files changed, 22 insertions(+), 4 deletions(-)
>>>
>>
>> Like the previous patch - is there a specific bug or something that led
>> you down this path? Can you show an example of the existing code that's
>> creating a bad command line and generating an error and then how this
>> fixes things. It's not like we have tests and for this stuff it's
>> really nice to have plenty of examples.
>
> So here is the run without my patches:
>
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
> iscsiadm: No active sessions.
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value
CHAP
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value
$USER
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value
$PASS
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-03316143 --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-03316143 --op update --name iface.initiatorname --value $INITIATOR
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type
sendtargets --portal $PORTAL:3260,1 --op nonpersistent
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143
> error : virCommandWait:2600 : internal error: Child process (iscsiadm --mode node
--portal $PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143)
unexpected exit status 21:
> iscsiadm: No records found
> iscsiadm: No records found
>
>
> And with my patches:
>
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
> iscsiadm: No active sessions.
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.authmethod --value
CHAP
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username --value
$USER
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password --value
$PASS
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-28727243 --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface --interface
libvirt-iface-28727243 --op update --name iface.initiatorname --value $INITIATOR
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery --type
sendtargets --portal $PORTAL:3260,1 --interface libvirt-iface-28727243
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal
$PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-28727243
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session -r 1 -R
>
>
So the example helps me understand - thanks! The difference of discovery
being:
fail: iscsiadm --mode discovery --type sendtargets \
--portal $PORTAL:3260,1 \
--op nonpersistent
succ: iscsiadm --mode discovery --type sendtargets \
--portal $PORTAL:3260,1 \
--interface libvirt-iface-28727243
So, what commit 56057900d did to "help" or "fix" auto-login for
sessions
that do not "belong to" libvirt is being called out as causing problems
for the initiatoriqn sessions. Prior to that commit the command was:
iscsiadm --mode discovery --type sendtargets --portal $PORTAL:3260,1
So, I have to wonder "how" or "if" discovery using an initiatoriqn
really worked since there wasn't an --interface argument.
Maybe it did. Maybe iscsiadm used to use all interfaces (which okay,
sounds unlikely) for sendtargets. Or it remembered that we added a
special interface for $PORTAL and preferred that. I have no idea. But it
is clearly not working now.
Not sure I
have the patience/desire to go through the history of refactors and
adjustments since 6aabcb5 though ;-). Although it does appear that the
original code would make a "node" during it's connection period rather
than doing a sendtargets using the --interface option (and whether that
option existed at the time is a different search through a different set
of code).
Given all of what's been discovered here - I think patch 4 and 5 should
be combined.
Well, I can merge them. It's just that I wanted these virISCSI* helpers
to be generic enough so that when one day somebody wants to persists
targets for default connection they can do that. I view @ifacename and
@persist arguments as orthogonal. It's only the usage of APIs that makes
the arguments mutually exclusive (or tied together or what).
And rather than adding a new "bool persist" parameter that
only is true when initiatoriqn is passed, let's use the fact that
initiatoriqn was passed in order to:
if (ifacename)
virCommandAddArgList(cmd, "--interface", ifacename, NULL);
else
virCommandAddArgList(cmd, "--op", "nonpersistent", NULL);
Then as long as non initiatoriqn sessions still work, even those without
a libvirt pool associated - we should be able to declare victory. Not
sure "if" non libvirt pool initiatoriqn sessions would be affected by
all this.
They shouldn't. That is why libvirt creates its own interface.
John
BTW: Based on that commit, I wonder if you can modify viriscsitest.c a
bit in order to generate the initiatoriqn output as well via a change to
testIscsiadmCb.
I'll look into it.
Michal
2364
days inactive
2368
days old
21 comments
4 participants
participants (4)
-
John Ferlan -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa