Re: [PATCH] ConnStateData flexible transport support

On 04/28/2014 07:10 AM, Amos Jeffries wrote:

> * ssl-bump transforms the transportVersion from whatever it was
> previously (usually HTTP or HTTPS) to HTTPS.

> + /// the transport protocol currently being spoken on this connection
> + AnyP::ProtocolVersion transportVersion;

If I recall our earlier discussions correctly, "transport" is TCP, UDP,
and such while HTTP and HTTPS are "transfer" protocols. It sounds like
you want to use the new data member for transfer, not transport
protocols. If yes, the member should be renamed accordingly.

Also, if this is often about the protocol name rather than just its
version, let's remove "Version" from the data member name (PortCfg
already uses that approach).

Finally, the patch does not actually use the version part of the
transportVersion data member AFAICT. Perhaps the data member type should
be changed from ProtocolVersion to ProtocolType?

> - debugs(33, 5, HERE << "converting " << clientConnection << " to SSL");
> + debugs(33, 5, "converting " << clientConnection << " to SSL");

To reduce merge conflicts, please do not remove HERE from debugging
lines that otherwise do not need to be changed.

> This variable can
> be altered whenever necessary to cause an on-wire protocol change.

Altering the data member does not cause an on-wire protocol change in
the patched code AFAICT. Perhaps you meant that the data member should
always reflect the current wire protocol?

Cheers,

Alex.
Received on Mon Apr 28 2014 - 21:12:37 MDT