ecki commented on code in PR #446:
URL: https://github.com/apache/mina-sshd/pull/446#discussion_r1435255754
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -151,24 +156,34 @@ public abstract class AbstractSession extends
SessionHelper {
protected final Map<KexProposalOption, String> clientProposal = new
EnumMap<>(KexProposalOption.class);
protected final Map<KexProposalOption, String> unmodClientProposal =
Collections.unmodifiableMap(clientProposal);
protected final Map<KexProposalOption, String> negotiationResult = new
EnumMap<>(KexProposalOption.class);
- protected final Map<KexProposalOption, String> unmodNegotiationResult =
Collections.unmodifiableMap(negotiationResult);
+ protected final Map<KexProposalOption, String> unmodNegotiationResult =
Collections
+ .unmodifiableMap(negotiationResult);
protected KeyExchange kex;
protected Boolean firstKexPacketFollows;
protected boolean initialKexDone;
+ protected final AtomicBoolean strictKexSignalled = new AtomicBoolean();
+ protected final AtomicInteger newKeysReceivedCount = new AtomicInteger();
+ protected final AtomicInteger newKeysSentCount = new AtomicInteger();
+
/**
* Holds the current key exchange state.
*/
protected final AtomicReference<KexState> kexState = new
AtomicReference<>(KexState.UNKNOWN);
protected final AtomicReference<DefaultKeyExchangeFuture> kexFutureHolder
= new AtomicReference<>(null);
- // The kexInitializedFuture is fulfilled when this side (client or server)
has prepared its own proposal. Access is
+ // The kexInitializedFuture is fulfilled when this side (client or server)
+ // has prepared its own proposal. Access is
// synchronized on kexState.
protected DefaultKeyExchangeFuture kexInitializedFuture;
/*
* SSH packets encoding / decoding support
*/
+ /** Input packet sequence number. */
+ protected long seqi;
+ /** Output packet sequence number. */
+ protected long seqo;
Review Comment:
Unfortunatelly your patch does modify unrelated things, it would be good not
to have to review them.
it is relevant to the patch because you introduced a synchronized and it was
not clear to me if it’s the right one (since it looks like the increase of
those variables do not know about it).
https://github.com/apache/mina-sshd/blob/f5c63a87d46ca5820e17af00b4e65a43288b58c7/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java#L1615
vs.
https://github.com/apache/mina-sshd/pull/446/files#diff-7341c56173d7ddcd7c2fd6e579e88c4878168a7bbb053157021c5980cdd1e68fR1266
resetSequenceNumbers synchronizes on strictKexSignalled.
yes it’s a complex code, but not documenting assumptions doesn’t make it
less complex. Anyway, you don’t need to add the guard when you are sure it’s
either correct or not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
