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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
> VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
>
> VIR_FREE(line);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
