netstat is discouraged in modern Linux; try ss first in Linux about cdc-file-transfer

[ljusten]

Nice, thanks, ss is even faster than netstat. You can also set --forward-port=some available port to skip the port detection altogether. That's actually a bit faster as well.

I'd like to get rid of the whole netstat calling logic and detect the ports in the remote components (e.g. cdc_rsync_server) in code, but that'll require some bigger refactorings since right now cdc_rsync_server and port forwarding are started at the same time.

[ljusten]

I'll replace the netstat command by

if which ss; then ss --numeric --listening --tcp; else netstat --numeric --listening --tcp; fi

[tats-u]

@ljusten remote_util->BuildProcessStartInfoForSsh can handle shell scripts, right?

cdc-file-transfer/common/port_manager_win.cc

Lines 235 to 283 in d8c2b59

	// static
	absl::StatusOr<std::unordered_set<int>> PortManager::FindAvailableRemotePorts(
	int first_port, int last_port, const char* ip,
	ProcessFactory* process_factory, RemoteUtil* remote_util, int timeout_sec,
	SteadyClock* steady_clock) {
	// --numeric to get numerical addresses.
	// --listening to get only listening sockets.
	// --tcp to get only TCP connections.
	std::string remote_command = "netstat --numeric --listening --tcp";
	ProcessStartInfo start_info =
	remote_util->BuildProcessStartInfoForSsh(remote_command);
	start_info.name = "netstat";
	start_info.flags = ProcessFlags::kNoWindow;
	std::string output;
	start_info.stdout_handler = [&output](const char* data, size_t data_size) {
	output.append(data, data_size);
	return absl::OkStatus();
	};
	std::string errors;
	start_info.stderr_handler = [&errors](const char* data, size_t data_size) {
	errors.append(data, data_size);
	return absl::OkStatus();
	};
	std::unique_ptr<Process> process = process_factory->Create(start_info);
	absl::Status status = process->Start();
	if (!status.ok()) return WrapStatus(status, "Failed to start netstat");
	Stopwatch timeout_timer(steady_clock);
	bool is_timeout = false;
	auto detect_timeout = [&timeout_timer, timeout_sec, &is_timeout]() {
	is_timeout = timeout_timer.ElapsedSeconds() > timeout_sec;
	return is_timeout;
	};
	status = process->RunUntil(detect_timeout);
	if (!status.ok()) return WrapStatus(status, "Failed to run netstat process");
	if (is_timeout)
	return absl::DeadlineExceededError("Timeout while running netstat");
	uint32_t exit_code = process->ExitCode();
	if (exit_code != 0) {
	return MakeStatus("netstat process exited with code %u:\n%s", exit_code,
	errors);
	}
	LOG_DEBUG("netstat (instance) output:\n%s", output);
	return FindAvailablePorts(first_port, last_port, output, ip);
	}

The candidate you have just show me may not work if the remote shell is fish or other shells incompatible with sh.
It's great if we can grab the status code of a one-shot command (e.g. which ss) in a single function call.

[ljusten]

That's true, but we already run similar code elsewhere, so at least it doesn't make the problem more severe. The real solution is to get rid of all shell scripts, but that's much more work.

cdc-file-transfer/cdc_rsync/cdc_rsync_client.cc

Line 210 in d8c2b59

absl::StrFormat("mkdir -p %s; if [ ! -f %s ]; then exit %i; fi; %s %i %s",

[ljusten]

Actually, this should work in a wider range of (all?) shells:

which ss && ss --numeric --listening --tcp || netstat --numeric --listening --tcp

It also has the advantage that it falls back to netstat if executing ss fails.

[tats-u]

@ljusten It has more portability. I'm afraid the ss's path vomited by which could occur unintended behavior.

$ which git && echo aaa || echo bbb
/usr/bin/git
aaa
$ which foo && echo aaa || echo bbb
bbb

Is it ignored when analyzed?

[ljusten]

Yes, it's ignored when analyzed. It can be hidden by adding > /dev/null after which ss, but I think it's a useful thing to have in debug logs since this way we know which one was executed.

[tats-u]

@ljusten I got it. I'll leave it to you. I've appreciated your quick response.

[tats-u]

I found missing netstat is treated as the connection timeout without -vvv.
This causes users' misunderstanding.

[tats-u]

@ljusten Thank you for the fix!