[
https://issues.apache.org/jira/browse/SSHD-1273?focusedWorklogId=787174&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-787174
]
ASF GitHub Bot logged work on SSHD-1273:
----------------------------------------
Author: ASF GitHub Bot
Created on: 01/Jul/22 17:16
Start Date: 01/Jul/22 17:16
Worklog Time Spent: 10m
Work Description: 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)`
Issue Time Tracking
-------------------
Worklog Id: (was: 787174)
Time Spent: 20m (was: 10m)
> Send environment variable and open subsystem at the same time for SSH session
> -----------------------------------------------------------------------------
>
> Key: SSHD-1273
> URL: https://issues.apache.org/jira/browse/SSHD-1273
> Project: MINA SSHD
> Issue Type: Improvement
> Affects Versions: 2.8.0
> Reporter: Andrei Danilenka
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> I use Apache Mina SSH client. The request that I want to send looks like:
> {code:java}
> user@host -w my_password -o SendEnv=SOME_VAR -s subsystem_name{code}
> But for now it is impossible to use env vars and subsystem together.
> What I have tried so far
> 1. ChannelShell class has setEnv(key, value) method but I can't go in
> subsystem
> 2. ChannelSubsystem class allows to go to subsystem but it has no
> setEnv(key, value) method
> 3. Also I have tried to open ChannelShell, set env, then open
> ChannelSubsystem using the same session as was used for ChannelShell, but it
> seems env variable hasn't gotten to ChannelSubsystem.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
