Re: [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);