[ 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: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org