Send SERVER_ERROR to client with error info in more cases

[davidBar-On]

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

• Issues fixed (if any): none

• Brief description of code changes (suitable for use as a commit message):

In both error_handling and cleanup_server() in iperf_server.c, added sending SERVER_ERROR state to the client with the error info (and removed now redundant SERVER_ERROR after parameters exchange).

Also added "SERVER ERROR: ..." error message in the client, when SERVER_ERROR state is received, so it will be clear that the error printed is from the server and not the client itself. (The current error message is printed by who ever calls iperf_run_client(), e.g. main.c, so could not just change that message.)

[bmah888]

Thanks for the PR! I think this looks like a good change, and in addition I appreciate factoring out the copy-paste code that sends the SERVER_ERROR over the control connection. I haven't yet tried testing compatibility with existing/old versions of iperf3, have you, or do you expect they should interoperate correctly?

[davidBar-On]

I haven't yet tried testing compatibility with existing/old versions of iperf3, have you, or do you expect they should interoperate correctly?

I did test with existing iperf3, but I don't expect an problem. Currently, when the server does not send SERVER_ERROR, the client can just identify that there is a connection problem. Therefore the worst case may be, if for some reason the client does not handle properly the state change, that its general error message will be changed.

[davidBar-On]

... have you, or do you expect they should interoperate correctly?

I have tested now with iperf3 that does not included the change and found that behavior is indeed o.k. However, I found that if a server error occurs in iperf_accept() the client does not get the new added error messages from the server, because the server immediately closes the control socket after sending the errors (in later execution phases the control socket is not closed immediately, because the data sockets are closed first). Therefore, I added shutdown(..., SHUT_WR) before closing the control socket. I added that also to the client.

From what I checked, almost all systems should support shutdown(..., SHUT_WR), but to make sure I added a check in confugure.ac for SHUT_WR. The alternative "wait for sync" method is sleep(1) which I belie is o.k. as practically it may not be in use. If this seems as an overkill for properly supporting server error reporting to the client during iperf_accept(), I can remove the SHUT_WR related changes.

[bmah888]

The functionality as far as error reporting looks good (from the first of the two commits in this PR).

For the second commit (the one that adds the shutdown() calls) I'm not sure that shutdown() before close() actually guarantees anything about the delivery of bytes previously sent from this host. I think if this adding this change works better as you described, it might be because the shutdown() and Nread() calls cause some delay (similar to what the sleep(1) does), but I'm not aware of anything in the TCP spec or the Linux TCP implementation about guaranteeing delivery.

I am tempted to say let's just go with the first commit, but I don't have any intuition how common is the problem you solve with the second commit. Any thoughts?

(If we do decide to keep the SHUT_WR code and checks we need to fix merge conflicts in configure.ac.)

[davidBar-On]

I'm not sure that shutdown() before close() actually guarantees anything about the delivery of bytes previously sent from this host.

My understanding that the shutdown(... SHUT_WR) does allow the client to receive the SERVER_ERROR messages. After receiving the messages, the client shutdown() (or close()) the socket which will cause the server Nread() to fail on EOF and then the server will close() the socket. Following is a more detailed expected behavior.

The shutdown(... SHUT_WR) from the server allows the client to receive and handle the SERVER_ERROR messages. Then, the client will shutdown(SHUT_WR) or close() the socket. This will cause the next read by the server to fail. This the reason for the while (Nread(sock, buffer, sizeof(buffer), 0) > 0); that follows the shutdown(). (Same is when the clients calls the shutdown().)

Even if the client does not receive the SERVER_ERROR messages, the client's select() will indicate that the socket is readable, because of the TCP FIN (sent from the server as a result of the shutdown(... SHUT_WR)). However, trying to read the socket will return EOF, which will cause the client to shutdown(SHUT_WR) or close() the socket.

If you agree keeping the shutdown() is OK than the change is probably ready for merge (I rebased and resolved the configure.ac conflict). If not, I will just keep the sleep(1) as you suggested.

[bmah888]

Should this comment read "received by the server"?

[davidBar-On]

Yes. Corrected.

[bmah888]

Thanks for fixing the merge conflict on configure.ac! I'm still pondering the behavior of shutdown(..., SHUT_WR) but I agree if it gives the behavior we want then this change is pretty much ready to be merged.

(Just had that one suggestion about a comment)

[davidBar-On]

Updated the comments (both for the client and server).

[bmah888]

Merged, thanks again!