fix exit value of iperf when using json format

[hajoha]

• Version of iperf3 (or development branch, such as master or
  3.1-STABLE) to which this pull request applies:
  since iperf3.11 & also master

• Issues fixed (if any):
  Have a look at Issue 1405

• Brief description of code changes (suitable for use as a commit message):
  change the cleanup_and_fail in iperf_client_api.c to

cleanup_and_fail:
    iperf_client_end(test);
    iflush(test);
    return -1;

[bmah888]

This code change has the desired effect in the iperf3 executable.

It's not clear to me how well the change works in the use cases of PR #1202 , where iperf_run_client is called as a library function. Specifically, your change means that the caller of iperf_run_client is responsible for populating the error member of the JSON structure. In the usual iperf3 program, that works fine, but I'm not sure if API consumers will know to do the right thing. Going to try to get some feedback from community members who were involved with #1202.

[bmah888]

@ludeeus and/or @matt9j : Would you be able to look at this PR and comment? It's trying make sure that iperf3 properly exits with a non-zero return code if there's an error in iperf_run_client. It seems to do that correctly, in that the iperf3 main function calls iperf_errexit, which properly populates the JSON, outputs it, and exits.

I'm not sure how well this change plays with API consumers. Other main programs using the iperf3 library (those that might want JSON output) would need to do something similar to what iperf3 does (calling iperf_errexit or at least iperf_err) in order to get an error message into the output JSON). Is that a reasonable assumption that other API consumers would do this?

Since you two had some involvement with this use case and this area of the code in PR #1202 I was wondering if you'd be able to take a look and give an opinion? Thanks!

[matt9j]

Hi @bmah888, I think that as an API consumer, it would be essential to have a set of well-defined and well-documented tasks that would need to happen if the function returned nonzero if there were going to need to be other cleanup steps run in the calling context.

Looking at the code in this particular case though, it looks like the JSON calls in iperf_errexit already check if the JSON object has been finalized and the pointer nulled, so it seems like there would not be an issue if iperf_client_api::run_client adds the json error string and finishes the json output, but then returns -1. iperf_errexit would need a slight tweak to not output to stdout or the file when in json mode, but this seems like a better solution to me than requiring API consumers to start making finalization calls on error (which would be a breaking API change). That has the added benefit that in both the success and the error case the json output is in the same state after the return of iperf_run_client from the API caller's perspective.

I can prepare a patch with this alternate proposal this weekend.

[bmah888]

Closing in favor of #1523, thanks for the submission!