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@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@redhat.com> John
VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
VIR_FREE(line);