Re: [PATCH v2 2/2] connect, protocol: log negotiated protocol version

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 5, 2021, 01:26 UTC
Message-ID: <YQs+VQIYDO3pkCNS@nand.local>
In-Reply-To: <87a6lwn52u.fsf@evledraar.gmail.com>

On Thu, Aug 05, 2021 at 01:40:51AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

>
> On Wed, Aug 04 2021, Josh Steadmon wrote:
>
> > It is useful for performance monitoring and debugging purposes to know
> > the wire protocol used for remote operations. This may differ from the
> > version set in local configuration due to differences in version and/or
> > configuration between the server and the client. Therefore, log the
> > negotiated wire protocol version via trace2, for both clients and
> > servers.
> >
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
>
> I know Taylor asked you to change it to a string from in int upthread in
> <YQmxSxTswHE/gTet@nand.local>, but I really don't see the point. But am
> willing to be convinced otherwise.

The conversion to log a string instead of an integer is necessary if Josh wants to write "<unknown>" instead of -1. To me, that seemed clearer, and I like that it makes the trace2 representation for a protocol version separate from the protocol_version enum.

Show 26 quoted lines

> It seems to me that both of these codepaths will never usefully use this
> new "UNKNOWN_VERSION" string you added, i.e.:
>
> >  connect.c                             |  3 +++
> >  protocol.c                            |  3 +++
> >  t/t5705-session-id-in-capabilities.sh | 12 ++++++++++++
> >  3 files changed, 18 insertions(+)
> >
> > diff --git a/connect.c b/connect.c
> > index 70b13389ba..5f0e113625 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -150,6 +150,9 @@ enum protocol_version discover_version(struct packet_reader *reader)
> >  		break;
> >  	}
> >
> > +	trace2_data_string("transfer", NULL, "negotiated-version",
> > +			   format_protocol_version(version));
>
> Right after this.
>
> >  	switch (version) {
> >  	case protocol_v2:
> >  		process_capabilities_v2(reader);
>
> We'll die here with BUG("unknown protocol version") if it's unknown..

Good eyes. In fact, the second switch statement shouldn't even need a case-arm for protocol_unknown_version (but has it to cover all enumerated values).

I didn't realize before that the unknown case really is dead code, so we'll never log "<unknown>". And since the mapping from protocol_version to string is identical for known values, we could probably do without it.

And I don't much care either way. I think the benefit is really pretty slim, and arguably my code is just adding unnecessary overhead. So I'm happy to go with or without it, but I'd be rather sad to spend much more of our collective time discussing it.

Thanks, Taylor