Annotate server output and logs with "title" from the client.

[berkakinci]

• 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 in functionality; new feature
  Typo in documentation.

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

I had a desire to annotate server output (and logs) from the client. The use case example:

1. I test my laptop (client) on wired network.
2. I test the same laptop a few minutes later from wireless network.

I would like to be able to identify what I was doing when browsing the server-side output. So I would like to leave a 'breadcrumb' (annotation) on the server invoked from the client.

This change accomplishes that. The client-set "--title" option now is also included on the Server output.
This parameter was already exchanged between server and client, so no major changes were necessary; just iperf_printf.

[berkakinci]

I just realized I forgot to update the JSON output.
I had looked at it, but it wasn't immediately clear how I should add it. May be you can fill that in?

[davidBar-On]

@berkakinci I think this is a good idea, so I have implemented your changes in a private iperf3 branch I maintain.

I have some comments/suggestions:

1. Regarding your question about JSON output, the title is already included in the JSON output by iperf_json_finish(), so it seems no related change is required.
2. Regarding test_start_title[], as it is printed by both client and server, it seems that it can be something like "Test title: %s\n".
3. Regarding printing test_start_title[] by iperf_on_test_start(), I think it should be done only in verbose mode, i.e. inside the if (test->verbose) {.
4. Is there a reason you didn't add the title to each server's output line? It can make it easier for identifying the test (although there may be a reason why it was implemented only for the client). In any case, I added that in my private branch. After:

   iperf/src/iperf_api.c

   Lines 4554 to 4559 in 53a6830

   	else if (test->role == 's') {
   	char linebuffer[1024];
   	int i = 0;
   	if (ct) {
   	i = sprintf(linebuffer, "%s", ct);
   	}

   
   I added:

        if (test->title)
	    i += sprintf(linebuffer + i, "%s:  ", test->title);

[berkakinci]

@davidBar-On, thanks for the feedback.

1. Regarding your question about JSON output, the title is already included in the JSON output by iperf_json_finish(), so it seems no related change is required.

Good.

2. Regarding test_start_title[], as it is printed by both client and server, it seems that it can be something like "Test title: %s\n".

That sounds great; less chatty.

3. Regarding printing test_start_title[] by iperf_on_test_start(), I think it should be done only in verbose mode, i.e. inside the if (test->verbose) {.

My coworker and I went back and forth on this. In the end we settled on output even if not verbose. We considered it vital identifying info assuming you took the time to set --title. So in our use case, it's about as important as the IP address of the connected client. So non-verbose is where we settled.

I don't know how others may be using --title, so that may not be desirable. (Do you know the other use cases for --title?)
In any case, this was just preference; I won't be heartbroken if you strongly feel it should remain in verbose mode. That's likely how we will demonize iperf3 anyway.

4. Is there a reason you didn't add the title to each server's output line? It can make it easier for identifying the test (although there may be a reason why it was implemented only for the client). In any case, I added that in my private branch.

(Side note: Those lines you quoted have buffer overflow hazards.)

Again, I don't know the existing use cases. I actually end up using pretty long and descriptive titles. It's kind of annoying (to me) to see those prepend every line even on the client side while doing the test. This would be the same on the server side.
I do agree It's great for grep to identify a specific test; I have no strong preference.

I piggybacked on --title because it was easy. The purist in me would like to make it a new parameter to exchange, but the lazy side doesn't want to change any more than necessary.

[davidBar-On]

We considered it vital identifying info assuming you took the time to set --title. So in our use case, it's about as important as the IP address of the connected client. So non-verbose is where we settled.

Regarding the above and:

I actually end up using pretty long and descriptive titles. It's kind of annoying (to me) to see those prepend every line even on the client side while doing the test. This would be the same on the server side.

I now understand your decision. If the title is not appended to each output line, then it is important to show it, not just in verbose mode. If the title is appended to each line, then that is not important. If and when the PR will be merged we will see what is the decision.

(Do you know the other use cases for --title?)

Sometimes I am running several clients in parallel from the same terminal, and the output from all clients are mixed. I also suggested a feature that allows one server to support several clients in parallel by executing a new server per client (PR #1074). In this case all servers outputs are displayed on the same terminal.

[berkakinci]

Between your --title use case and ours, this compromise comes to mind:

• The "Test title:" line shows up regardless of verbosity
• The title is prepended to server output on every line in verbose mode.
  It's easy enough to filter the log later.

However, my conscience is pushing me away from touching that iperf_printf() function. I can easily see sending a long --title to overflow that 1024 character buffer. There is no check for ct or test->title. Both can overflow linebuffer[1024]. Even worse, test->title would be coming from the client, not set by the server's invocation.

I looked around that iperf_printf() some more. It's not as bad as the quoted code above suggests. I guess it's reasonable to fix those problems in that area too.

[berkakinci]

So, I think I'm going to need a little guidance from maintainers on this one.
The "The title is prepended to server output on every line in verbose mode" of this PR and fix for buffer overflow Issue #1134 both will touch the same lines.
Should I go ahead and make possibly non-automatically-merging edits on two PRs? Lump the buffer overflow fix into this PR? Wait for the more-urgent buffer overflow bug to get resolved then rebase?

[davidBar-On]

This is probably a question that @bmah888 can give the real answer, but from my experience it is better to keep these two issues separate, as they are not directly related. In general it is better to keep each PR as simple as possible.

[bmah888]

@berkakinci Apologies for the delay, I'm starting to look at iperf3 stuff now after a hiatus caused by other work priorities.

In generally it's better two do separate, smaller fixes, although I haven't really examined #1133 or #1134 yet so I can't really comment on the specifics of those two changes.