lgoldstein commented on code in PR #230:
URL: https://github.com/apache/mina-sshd/pull/230#discussion_r912124733
##########
sshd-core/src/main/java/org/apache/sshd/client/channel/ChannelSession.java:
##########
@@ -48,6 +53,7 @@ public class ChannelSession extends AbstractClientChannel {
private CloseableExecutorService pumperService;
private Future<?> pumper;
+ private Map<String, Object> env = new LinkedHashMap<>();
Review Comment:
Should be `final` - unless there is some reason to make it mutabel (?)
##########
sshd-core/src/main/java/org/apache/sshd/client/channel/ChannelSession.java:
##########
@@ -217,4 +223,42 @@ protected int securedRead(
}
}
}
+
+ /**
+ * @param key The (never {@code null}) key (Note: may be empty...)
+ * @param value The value to set - if {@code null} then the pre-existing
value for the key (if any) is
+ * <U>removed</U>.
+ * @return The replaced/removed previous value - {@code null} if no
previous value set for the key.
+ */
+ public Object setEnv(String key, Object value) {
+ ValidateUtils.checkNotNull(key, "No key provided");
+ if (value == null) {
+ return env.remove(key);
+ } else {
+ return env.put(key, value);
+ }
+ }
+
+ protected void sendEnvVariables(Session session) throws IOException {
+ if (MapEntryUtils.size(env) > 0) {
+ if (log.isDebugEnabled()) {
+ log.debug("Sending env variables ({}) Send
SSH_MSG_CHANNEL_REQUEST env: {}", this, env);
+ }
+
+ // Cannot use forEach because of the IOException being thrown by
writePacket
+ for (Map.Entry<String, ?> entry : env.entrySet()) {
+ String key = entry.getKey();
+ Object value = entry.getValue();
+ String str = Objects.toString(value);
+ Buffer buffer = session.createBuffer(
+ SshConstants.SSH_MSG_CHANNEL_REQUEST, key.length() +
GenericUtils.length(str) + Integer.SIZE);
+ buffer.putInt(getRecipient());
+ buffer.putString("env");
+ buffer.putBoolean(false); // want-reply
Review Comment:
The standard does not say that *wantReply* must be *false* - while I do
agree that it is a sensible default have you given thought how yo handle *true*
? At the very least this choice should be documented and perhaps open a
follow-up issue to add support for *wantReply=true* in some future release
##########
sshd-core/src/main/java/org/apache/sshd/client/channel/PtyCapableChannelSession.java:
##########
@@ -289,25 +273,6 @@ protected void doOpenPty() throws IOException {
writePacket(buffer);
}
- if (MapEntryUtils.size(env) > 0) {
- if (debugEnabled) {
- log.debug("doOpenPty({}) Send SSH_MSG_CHANNEL_REQUEST env:
{}", this, env);
- }
-
- // Cannot use forEach because of the IOException being thrown by
writePacket
- for (Map.Entry<String, ?> entry : env.entrySet()) {
- String key = entry.getKey();
- Object value = entry.getValue();
- String str = Objects.toString(value);
- Buffer buffer = session.createBuffer(
- SshConstants.SSH_MSG_CHANNEL_REQUEST, key.length() +
GenericUtils.length(str) + Integer.SIZE);
- buffer.putInt(getRecipient());
- buffer.putString("env");
- buffer.putBoolean(false); // want-reply
Review Comment:
See previous comment regarding *wantReply=false* as hardwired value
##########
sshd-core/src/main/java/org/apache/sshd/client/channel/ChannelSession.java:
##########
@@ -217,4 +223,42 @@ protected int securedRead(
}
}
}
+
+ /**
+ * @param key The (never {@code null}) key (Note: may be empty...)
+ * @param value The value to set - if {@code null} then the pre-existing
value for the key (if any) is
+ * <U>removed</U>.
+ * @return The replaced/removed previous value - {@code null} if no
previous value set for the key.
+ */
+ public Object setEnv(String key, Object value) {
+ ValidateUtils.checkNotNull(key, "No key provided");
Review Comment:
Please use `ValidateUtils#checkNotNullAndNotEmpty(key)`
##########
sshd-core/src/main/java/org/apache/sshd/client/channel/ChannelSession.java:
##########
@@ -48,6 +53,7 @@ public class ChannelSession extends AbstractClientChannel {
private CloseableExecutorService pumperService;
private Future<?> pumper;
+ private Map<String, Object> env = new LinkedHashMap<>();
Review Comment:
Is there any special reason why you are using a `LinkedHashMap` rather than
a `HashMap` ? Does the order of the keys matter in any way ?
##########
sshd-core/src/main/java/org/apache/sshd/client/channel/PtyCapableChannelSession.java:
##########
@@ -200,21 +199,6 @@ public void setPtyModes(Map<PtyMode, Integer> ptyModes) {
config.setPtyModes((ptyModes == null) ? Collections.emptyMap() :
ptyModes);
}
- /**
- * @param key The (never {@code null}) key (Note: may be empty...)
- * @param value The value to set - if {@code null} then the pre-existing
value for the key (if any) is
- * <U>removed</U>.
- * @return The replaced/removed previous value - {@code null} if no
previous value set for the key.
- */
- public Object setEnv(String key, Object value) {
- ValidateUtils.checkNotNull(key, "No key provided");
Review Comment:
Please use `ValidateUtils#checkNotNullAndNotEmpty(key)`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
