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

> On Tue, Aug 03, 2021 at 01:13:02PM -0700, Josh Steadmon wrote:
> > [...] Therefore, log the negotiated wire protocol version via trace2,
> > for both clients and servers.
> 
> Seems useful, thanks.
> 
> > 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.

> > 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.

> (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...
> 
> > 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.