Re: [PATCH] connect, protocol: log negotiated protocol version

On 2021.08.03 17:12, Taylor Blau wrote:

On Tue, Aug 03, 2021 at 01:13:02PM -0700, Josh Steadmon wrote:

quoted

[...] Therefore, log the negotiated wire protocol version via trace2,
for both clients and servers.

Seems useful, thanks.

quoted

Do people have a preference for logging this as an integer (and
therefore having "unknown protocol version" show up as "-1", or should I
add a protocol_version_to_string function so that we can format it
properly? For now I've erred on the side of having a smaller diff.

I probably have a slight preference for converting the protocol_version
to a string and passing that along to trace2_data_string() instead. That
would let you more cleanly log "<unknown>", without needing to expose
implementation details like which enum has what value.

V2 will include a format_protocol_version().

quoted

diff --git a/connect.c b/connect.c
index 70b13389ba..6e23e3ff2d 100644
--- a/connect.c
+++ b/connect.c

@@ -164,6 +164,7 @@ enum protocol_version discover_version(struct packet_reader *reader)
 		BUG("unknown protocol version");
 	}

+	trace2_data_intmax("transfer", NULL, "negotiated-version", version);

Small nit-pick, this could come between the two switch statements, not
at the end of the function (since we know what we are going to return
before we switch on version.

Fixed in V2.

(I was a little surprised to see that these functions now have the
side-effect of writing to trace2, since I would have instead expected
to see new lines added at the callers. But this makes it more
convenient, and I do not feel strongly about it)

In any case, connect.c:discover_version() is handling the client side,
but...

quoted

diff --git a/protocol.c b/protocol.c
index 052d7edbb9..3791d8499e 100644
--- a/protocol.c
+++ b/protocol.c

@@ -89,5 +89,6 @@ enum protocol_version determine_protocol_version_client(const char *server_respo
 			die("protocol error: server explicitly said version 0");
 	}

+	trace2_data_intmax("transfer", NULL, "negotiated-version", version);
 	return version;
 }
--

This function is used by discover_version to parse the server's
response. If you are trying to log what protocol was agreed on from the
server's perspective, I think you are looking instead for
determine_protocol_version_server().

If you aren't (and are only interested in the client's point-of-view),
then I am pretty sure that this latter hunk is redundant.

Thanks for catching this dumb mistake. Fixed in V2, and added tests to
make sure the trace message shows up on both client- & server-side.

Thanks,
Taylor