[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411695349
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelAsyncOutputStream.java
##
@@ -107,9 +108,39 @@ protected synchronized void doWriteIfPossible(boolean
resume) {
if (total > 0) {
Channel channel = getChannel();
Window remoteWindow = channel.getRemoteWindow();
-long length = Math.min(Math.min(remoteWindow.getSize(), total),
remoteWindow.getPacketSize());
-if (log.isTraceEnabled()) {
-log.trace("doWriteIfPossible({})[resume={}] attempting to
write {} out of {}", this, resume, length, total);
+long length;
+if (remoteWindow.getSize() < total && total <=
remoteWindow.getPacketSize()) {
+// do not chunk when the window is smaller than the packet size
+length = 0;
+// do a defensive copy in case the user reuses the buffer
+IoWriteFutureImpl f = new IoWriteFutureImpl(future.getId(),
new ByteArrayBuffer(buffer.getCompactData()));
+f.addListener(w -> future.setValue(w.getException() != null ?
w.getException() : w.isWritten()));
+pendingWrite.set(f);
+if (log.isTraceEnabled()) {
+log.trace("doWriteIfPossible({})[resume={}] waiting for
window space {}", this, resume,
+remoteWindow.getSize());
+}
+} else if (total > remoteWindow.getPacketSize()) {
+if (buffer.rpos() > 0) {
+// do a defensive copy in case the user reuses the buffer
Review comment:
Agreed, I'll split it somehow.
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.
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411695054
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##
@@ -954,7 +954,9 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws
IOException {
synchronized (encodeLock) {
Buffer packet = resolveOutputPacket(buffer);
IoSession networkSession = getIoSession();
-return networkSession.writePacket(packet);
+IoWriteFuture future = networkSession.writePacket(packet);
+buffer.rpos(buffer.wpos());
+return future;
Review comment:
The code is the following:
```
synchronized (encodeLock) {
Buffer packet = resolveOutputPacket(buffer);
IoSession networkSession = getIoSession();
IoWriteFuture future = networkSession.writePacket(packet);
buffer.rpos(buffer.wpos());
return future;
}
```
The `buffer` is actually read from within the call to `resolveOutputPacket`
and encoded into the `packet` variable. So while the `packet` is yet to be
written, the `buffer` won't be used anymore and is consumed at the time the
`buffer.rpos(buffer.wpos())` is called.
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.
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123: URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411694928 ## File path: sshd-sftp/pom.xml ## @@ -85,6 +85,18 @@ jzlib test + +org.testcontainers +testcontainers +1.12.5 +test + + +org.testcontainers +toxiproxy +1.12.5 +test + Review comment: Thx, I'll go for the BOM then... 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. 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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411692176
##
File path:
sshd-sftp/src/test/java/org/apache/sshd/client/subsystem/sftp/SftpTransferTest.java
##
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.client.subsystem.sftp;
+
+import java.io.BufferedWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.Date;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.client.subsystem.sftp.fs.SftpFileSystem;
+import org.junit.Test;
+
+public class SftpTransferTest extends AbstractSftpClientTestSupport {
Review comment:
From the `@FixMethodOrder` javadoc:
> the order of execution was changed to be deterministic (in JUnit 4.11)
I'll add it, but not sure when it's actually useful...
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.
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411689843
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/SftpRemotePathChannel.java
##
@@ -524,13 +527,13 @@ protected void endBlocking(Object actionHint, boolean
completed)
* @param reqModesThe required modes - ignored if {@code null}/empty
* @throws IOException If channel not open or the required modes are not
satisfied
*/
-private void ensureOpen(Collection reqModes) throws
IOException {
+private void ensureOpen(Collection reqModes) throws IOException {
Review comment:
Sure, but that's not part of my changes, so I'd rather use a different
commit to fix issues in the existing code rather than mixing both.
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.
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411687971
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/SftpRemotePathChannel.java
##
@@ -346,30 +355,19 @@ public long transferTo(long position, long count,
WritableByteChannel target) th
}
boolean completed = false;
-boolean eof = false;
-long curPos = position;
-int bufSize = (int) Math.min(count, copySize);
-byte[] buffer = new byte[bufSize];
-long totalRead = 0L;
+boolean eof;
+long totalRead;
synchronized (lock) {
try {
beginBlocking("transferTo");
-while (totalRead < count && !eof) {
-int read = sftp.read(handle, curPos, buffer, 0,
-(int) Math.min(count - totalRead, buffer.length));
-if (read > 0) {
-ByteBuffer wrap = ByteBuffer.wrap(buffer, 0, read);
-while (wrap.remaining() > 0) {
-target.write(wrap);
-}
-curPos += read;
-totalRead += read;
-} else {
-eof = read == -1;
-}
-}
+SftpInputStreamAsync input = new SftpInputStreamAsync(
+(AbstractSftpClient) sftp,
+copySize, position, count, getRemotePath(), handle);
Review comment:
Calling `close` has 2 effects: blocking until the stream is fully
consumed and closing the handle. In this particular usage, the data is already
consumed in the `transferTo` and we don't want to close the handle. The
`close` implementation is really focused on the standard usage of the
`InputStream`, not this internal usage.
If that really sounds too hacky, we can always use two different classes
through inheritance, but I thought the comment would be sufficient.
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.
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411688147
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/SftpRemotePathChannel.java
##
@@ -410,18 +408,23 @@ public long transferFrom(ReadableByteChannel src, long
position, long count) thr
try {
beginBlocking("transferFrom");
+SftpOutputStreamAsync output = new SftpOutputStreamAsync(
+(AbstractSftpClient) sftp,
+copySize, getRemotePath(), handle);
Review comment:
Same as above.
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.
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
[GitHub] [mina-sshd] gnodet commented on a change in pull request #123: SFTP transfer speeds, clean API
gnodet commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411682132
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/AbstractSftpClient.java
##
@@ -783,10 +786,6 @@ public void rename(String oldPath, String newPath,
Collection options)
@Override
public int read(Handle handle, long fileOffset, byte[] dst, int dstOffset,
int len, AtomicReference eofSignalled)
throws IOException {
-if (eofSignalled != null) {
-eofSignalled.set(null);
-}
-
Review comment:
We have `read` which calls `checkData` which calls `checkDataResponse`.
Having the 3 methods setting `eofSignalled` to null when entering while the
only place where it can be set to a non null value is later inside the
`checkDataResponse` seems a bit redundant to me. Also, I think checking the
EOF value in case an exception has been thrown is meaningless, so only keeping
the `eofSignalled.set(null)` from inside `checkDataResponse` sounds enough to
me.
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/AbstractSftpClient.java
##
@@ -802,9 +801,6 @@ public int read(Handle handle, long fileOffset, byte[] dst,
int dstOffset, int l
protected int checkData(
int cmd, Buffer request, int dstOffset, byte[] dst,
AtomicReference eofSignalled)
throws IOException {
-if (eofSignalled != null) {
-eofSignalled.set(null);
-}
Review comment:
See above
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.
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
Re: Autoformat source code issue
The plugin is definitely supposed to handle all line endings kind. I've boot up a windows 10 VM, cloned the git repo, built the whole sshd project. modified: sshd-common/src/main/java/org/apache/sshd/common/cipher/package.html modified: sshd-common/src/main/java/org/apache/sshd/common/compression/package.html modified: sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCrypt.java modified: sshd-common/src/main/java/org/apache/sshd/common/digest/package.html modified: sshd-common/src/main/java/org/apache/sshd/common/mac/package.html modified: sshd-common/src/main/java/org/apache/sshd/common/random/package.html modified: sshd-common/src/main/java/org/apache/sshd/common/signature/package.html So that does not look so terrible at this point. I suppose if you check out a clean repo, you should have the same, and those should be fixed anyway. When working on an existing repo and modifying the .gitattributes, you may need to use the --renormalize option as indicated in https://help.github.com/en/github/using-git/configuring-git-to-handle-line-endings#refreshing-a-repository-after-changing-line-endings to fix the line endings. After running git add --renormalize . , I only have the BCrypt.java file which definitely has incorrect line endings. Could you try a clean repo and/or using the --renormalize option ? Le lun. 20 avr. 2020 à 20:46, Jonathan Valliere a écrit : > Did you delete the whole repo and re-clone it? I have no idea, I don’t use > Windows for anything relating to work. > > On Mon, Apr 20, 2020 at 1:49 PM Lyor Goldstein > wrote: > > > >> >> > > https://stackoverflow.com/questions/21822650/disable-git-eol-conversions > > >> >> You will probably have to checkout the repo again. > > > > >> Thx - I'll give it a try... > > > > No good - tried all sort of combinations for .gitconfig and > .gitattributes > > but none worked quite frustrating - makes no sense to commit 100's of > > "changed" files - not to mention not being able to distinguish between > real > > changes and CRLF ones Feels like this auto-formatting is proving more > > trouble than benefit... > > > -- Guillaume Nodet
Re: Autoformat source code issue
Did you delete the whole repo and re-clone it? I have no idea, I don’t use Windows for anything relating to work. On Mon, Apr 20, 2020 at 1:49 PM Lyor Goldstein wrote: > >> >> > https://stackoverflow.com/questions/21822650/disable-git-eol-conversions > >> >> You will probably have to checkout the repo again. > > >> Thx - I'll give it a try... > > No good - tried all sort of combinations for .gitconfig and .gitattributes > but none worked quite frustrating - makes no sense to commit 100's of > "changed" files - not to mention not being able to distinguish between real > changes and CRLF ones Feels like this auto-formatting is proving more > trouble than benefit... >
Re: Autoformat source code issue
>> >> https://stackoverflow.com/questions/21822650/disable-git-eol-conversions >> >> You will probably have to checkout the repo again. >> Thx - I'll give it a try... No good - tried all sort of combinations for .gitconfig and .gitattributes but none worked quite frustrating - makes no sense to commit 100's of "changed" files - not to mention not being able to distinguish between real changes and CRLF ones Feels like this auto-formatting is proving more trouble than benefit...
[GitHub] [mina-sshd] FliegenKLATSCH commented on issue #122: Allow colon as valid host pattern char to support ipv6 entries in kno…
FliegenKLATSCH commented on issue #122: URL: https://github.com/apache/mina-sshd/pull/122#issuecomment-616704666 Should be fine now. Do you already have any plannings regarding the next release (date)? 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. 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
Re: Autoformat source code issue
>> https://stackoverflow.com/questions/21822650/disable-git-eol-conversions >> You will probably have to checkout the repo again. Thx - I'll give it a try...
Re: Autoformat source code issue
https://stackoverflow.com/questions/21822650/disable-git-eol-conversions You will probably have to checkout the repo again. On Mon, Apr 20, 2020 at 1:23 PM Lyor Goldstein wrote: > >> Maven auto format might be converting it back to LF then GIT gets > confused? Maybe just disable the conversion to CRLF? > > How do I do that ? >
Re: Autoformat source code issue
>> Maven auto format might be converting it back to LF then GIT gets confused? Maybe just disable the conversion to CRLF? How do I do that ?
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123: URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411554606 ## File path: sshd-sftp/pom.xml ## @@ -85,6 +85,18 @@ jzlib test + +org.testcontainers +testcontainers +1.12.5 +test + + +org.testcontainers +toxiproxy +1.12.5 +test + Review comment: Even better... 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. 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
[GitHub] [mina-sshd] bsideup commented on a change in pull request #123: SFTP transfer speeds, clean API
bsideup commented on a change in pull request #123: URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411495180 ## File path: sshd-sftp/pom.xml ## @@ -85,6 +85,18 @@ jzlib test + +org.testcontainers +testcontainers +1.12.5 +test + + +org.testcontainers +toxiproxy +1.12.5 +test + Review comment: there is also a BOM: `org.testcontainers:testcontainers-bom:1.14.0` See https://www.testcontainers.org/#managing-versions-for-multiple-testcontainers-dependencies 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. 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
Re: Autoformat source code issue
Maven auto format might be converting it back to LF then GIT gets confused? Maybe just disable the conversion to CRLF? On Mon, Apr 20, 2020 at 11:03 AM Lyor Goldstein wrote: > >> I thought GIT does this automatically on Windows. > > Not entirely - there are some core settings that control it, but it is not > clear how they affect the behavior. My current setup in .gitconfig is shown > below: > > [core] > autocrlf = input > filemode = false > > > I have been using it so far without any problems - it is only after the > Maven auto-formatting plugin was added that I am encountering problems. >
Re: Autoformat source code issue
>> I thought GIT does this automatically on Windows. Not entirely - there are some core settings that control it, but it is not clear how they affect the behavior. My current setup in .gitconfig is shown below: [core] autocrlf = input filemode = false I have been using it so far without any problems - it is only after the Maven auto-formatting plugin was added that I am encountering problems.
Re: Autoformat source code issue
I thought GIT does this automatically on Windows. On Mon, Apr 20, 2020 at 10:30 AM Lyor Goldstein wrote: > I have just fetched the latest master branch and built it on my Windows > machine and am getting as if all files have been changed - the change has > to do with LF -> CRLF change in all files. How do we prevent this from > happening? Personally I prefer LF, but let's make sure that building on > Linux or Windows does not affect the EOL... >
[jira] [Commented] (DIRMINA-1122) Add support for endpoint identification algorithm
[ https://issues.apache.org/jira/browse/DIRMINA-1122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17087789#comment-17087789 ] Marcin L commented on DIRMINA-1122: --- Cool. You commented about the same time I submitted the pull request. > Add support for endpoint identification algorithm > - > > Key: DIRMINA-1122 > URL: https://issues.apache.org/jira/browse/DIRMINA-1122 > Project: MINA > Issue Type: Improvement > Components: Filter, SSL >Affects Versions: 2.0.22, 2.1.3 >Reporter: Marcin L >Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > Support for endpoint identification algorithm was added in Java 1.7. > Currently MINA supports providing single SNI name via > org.apache.mina.filter.ssl.SslFilter#PEER_ADDRESS session attribute, but > there is no way verifying it matches the certificate received. > It would be nice if we could provide endpoint identification algorithm to > SslFilter so certificate's common name or subject alternative names are > verified. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
Autoformat source code issue
I have just fetched the latest master branch and built it on my Windows machine and am getting as if all files have been changed - the change has to do with LF -> CRLF change in all files. How do we prevent this from happening? Personally I prefer LF, but let's make sure that building on Linux or Windows does not affect the EOL...
[jira] [Commented] (DIRMINA-1122) Add support for endpoint identification algorithm
[ https://issues.apache.org/jira/browse/DIRMINA-1122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17087782#comment-17087782 ] Emmanuel Lécharny commented on DIRMINA-1122: Actually, no, and I just checked it. Sounds reasonable to me to add such a feature. > Add support for endpoint identification algorithm > - > > Key: DIRMINA-1122 > URL: https://issues.apache.org/jira/browse/DIRMINA-1122 > Project: MINA > Issue Type: Improvement > Components: Filter, SSL >Affects Versions: 2.0.22, 2.1.3 >Reporter: Marcin L >Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > Support for endpoint identification algorithm was added in Java 1.7. > Currently MINA supports providing single SNI name via > org.apache.mina.filter.ssl.SslFilter#PEER_ADDRESS session attribute, but > there is no way verifying it matches the certificate received. > It would be nice if we could provide endpoint identification algorithm to > SslFilter so certificate's common name or subject alternative names are > verified. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] FliegenKLATSCH commented on issue #119: Add support for openssh host key certificates
FliegenKLATSCH commented on issue #119: URL: https://github.com/apache/mina-sshd/pull/119#issuecomment-616590045 Ok, thanks. Changelog might just be misleading. 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. 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
[jira] [Resolved] (SSHD-660) Add support for authentication using signed client/server keys
[ https://issues.apache.org/jira/browse/SSHD-660?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Lyor Goldstein resolved SSHD-660. - Fix Version/s: 2.4.1 Resolution: Fixed > Add support for authentication using signed client/server keys > -- > > Key: SSHD-660 > URL: https://issues.apache.org/jira/browse/SSHD-660 > Project: MINA SSHD > Issue Type: Improvement >Reporter: Lyor Goldstein >Priority: Minor > Fix For: 2.4.1 > > > Similar to _HostCertificate_ and _TrustedUserCAKeys_ configuration values - > see https://ef.gy/hardening-ssh -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #123: SFTP transfer speeds, clean API
lgoldstein commented on a change in pull request #123:
URL: https://github.com/apache/mina-sshd/pull/123#discussion_r411387849
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelAsyncOutputStream.java
##
@@ -107,9 +108,39 @@ protected synchronized void doWriteIfPossible(boolean
resume) {
if (total > 0) {
Channel channel = getChannel();
Window remoteWindow = channel.getRemoteWindow();
-long length = Math.min(Math.min(remoteWindow.getSize(), total),
remoteWindow.getPacketSize());
-if (log.isTraceEnabled()) {
-log.trace("doWriteIfPossible({})[resume={}] attempting to
write {} out of {}", this, resume, length, total);
+long length;
+if (remoteWindow.getSize() < total && total <=
remoteWindow.getPacketSize()) {
+// do not chunk when the window is smaller than the packet size
+length = 0;
+// do a defensive copy in case the user reuses the buffer
+IoWriteFutureImpl f = new IoWriteFutureImpl(future.getId(),
new ByteArrayBuffer(buffer.getCompactData()));
+f.addListener(w -> future.setValue(w.getException() != null ?
w.getException() : w.isWritten()));
+pendingWrite.set(f);
+if (log.isTraceEnabled()) {
+log.trace("doWriteIfPossible({})[resume={}] waiting for
window space {}", this, resume,
+remoteWindow.getSize());
+}
+} else if (total > remoteWindow.getPacketSize()) {
+if (buffer.rpos() > 0) {
+// do a defensive copy in case the user reuses the buffer
Review comment:
This is a lot of code in a single method - can we somehow separate each
case to a (protected) method - easier to read, debug, maintain...
##
File path:
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##
@@ -954,7 +954,9 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws
IOException {
synchronized (encodeLock) {
Buffer packet = resolveOutputPacket(buffer);
IoSession networkSession = getIoSession();
-return networkSession.writePacket(packet);
+IoWriteFuture future = networkSession.writePacket(packet);
+buffer.rpos(buffer.wpos());
+return future;
Review comment:
Doesn't this change the `rpos` while the session is writing the buffer ?
Feels like this should be done only **after** packet has been written
##
File path: sshd-sftp/pom.xml
##
@@ -85,6 +85,18 @@
jzlib
test
+
+org.testcontainers
+testcontainers
+1.12.5
+test
+
+
+org.testcontainers
+toxiproxy
+1.12.5
+test
+
Review comment:
If these 2 dependencies are always expected to have the same version
let's use a property (e.g., `${testcontainers.version}` - this way if we
upgrade one we will not forget to update the other (D.R.Y. principle)
##
File path:
sshd-sftp/src/main/java/org/apache/sshd/client/subsystem/sftp/impl/SftpOutputStreamAsync.java
##
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.client.subsystem.sftp.impl;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Deque;
+import java.util.LinkedList;
+import java.util.Objects;
+
+import org.apache.sshd.client.subsystem.sftp.SftpClient;
+import org.apache.sshd.client.subsystem.sftp.SftpClient.CloseableHandle;
+import org.apache.sshd.client.subsystem.sftp.SftpClient.OpenMode;
+import org.apache.sshd.common.SshConstants;
+import org.apache.sshd.common.subsystem.sftp.SftpConstants;
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.io.OutputStreamWithChannel;
+
+/**
+ * Implements an output stream for a given remote file
+ *
+ * @author mailto:dev@mina.apache.org";>Apache MINA SSHD Projec
[jira] [Commented] (DIRMINA-1122) Add support for endpoint identification algorithm
[ https://issues.apache.org/jira/browse/DIRMINA-1122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17087771#comment-17087771 ] Marcin L commented on DIRMINA-1122: --- Yes - you probably can. The problem is to support all the features with wildcard matching will take quite a bit of development and testing. Java provides these features out of the box, why not use them? I assume you looked at the pull request. > Add support for endpoint identification algorithm > - > > Key: DIRMINA-1122 > URL: https://issues.apache.org/jira/browse/DIRMINA-1122 > Project: MINA > Issue Type: Improvement > Components: Filter, SSL >Affects Versions: 2.0.22, 2.1.3 >Reporter: Marcin L >Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > Support for endpoint identification algorithm was added in Java 1.7. > Currently MINA supports providing single SNI name via > org.apache.mina.filter.ssl.SslFilter#PEER_ADDRESS session attribute, but > there is no way verifying it matches the certificate received. > It would be nice if we could provide endpoint identification algorithm to > SslFilter so certificate's common name or subject alternative names are > verified. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
[jira] [Comment Edited] (DIRMINA-1122) Add support for endpoint identification algorithm
[ https://issues.apache.org/jira/browse/DIRMINA-1122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17087766#comment-17087766 ] Emmanuel Lécharny edited comment on DIRMINA-1122 at 4/20/20, 2:08 PM: -- I believe this can be done through your own implementation of a TrustManager that you inject in the SSLContext instance. was (Author: elecharny): I believe this can be done through your own implementation of a TrsutManager that you inject in the SSLContext instance. > Add support for endpoint identification algorithm > - > > Key: DIRMINA-1122 > URL: https://issues.apache.org/jira/browse/DIRMINA-1122 > Project: MINA > Issue Type: Improvement > Components: Filter, SSL >Affects Versions: 2.0.22, 2.1.3 >Reporter: Marcin L >Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > Support for endpoint identification algorithm was added in Java 1.7. > Currently MINA supports providing single SNI name via > org.apache.mina.filter.ssl.SslFilter#PEER_ADDRESS session attribute, but > there is no way verifying it matches the certificate received. > It would be nice if we could provide endpoint identification algorithm to > SslFilter so certificate's common name or subject alternative names are > verified. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
[jira] [Commented] (DIRMINA-1122) Add support for endpoint identification algorithm
[ https://issues.apache.org/jira/browse/DIRMINA-1122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17087766#comment-17087766 ] Emmanuel Lécharny commented on DIRMINA-1122: I believe this can be done through your own implementation of a TrsutManager that you inject in the SSLContext instance. > Add support for endpoint identification algorithm > - > > Key: DIRMINA-1122 > URL: https://issues.apache.org/jira/browse/DIRMINA-1122 > Project: MINA > Issue Type: Improvement > Components: Filter, SSL >Affects Versions: 2.0.22, 2.1.3 >Reporter: Marcin L >Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > Support for endpoint identification algorithm was added in Java 1.7. > Currently MINA supports providing single SNI name via > org.apache.mina.filter.ssl.SslFilter#PEER_ADDRESS session attribute, but > there is no way verifying it matches the certificate received. > It would be nice if we could provide endpoint identification algorithm to > SslFilter so certificate's common name or subject alternative names are > verified. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
[jira] [Work logged] (DIRMINA-1122) Add support for endpoint identification algorithm
[ https://issues.apache.org/jira/browse/DIRMINA-1122?focusedWorklogId=425327&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-425327 ] ASF GitHub Bot logged work on DIRMINA-1122: --- Author: ASF GitHub Bot Created on: 20/Apr/20 13:47 Start Date: 20/Apr/20 13:47 Worklog Time Spent: 10m Work Description: the-thing opened a new pull request #26: URL: https://github.com/apache/mina/pull/26 https://issues.apache.org/jira/projects/DIRMINA/issues/DIRMINA-1122 We found out that it is not possible to set endpoint identification algorithm while discussing SNI support in quickfixj https://github.com/quickfix-j/quickfixj/issues/276 **Changes** - added support for endpoint identification algorithm to SslFilter - unit tests for SNI CN/SAN certificate matching 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 425327) Remaining Estimate: 0h Time Spent: 10m > Add support for endpoint identification algorithm > - > > Key: DIRMINA-1122 > URL: https://issues.apache.org/jira/browse/DIRMINA-1122 > Project: MINA > Issue Type: Improvement > Components: Filter, SSL >Affects Versions: 2.0.22, 2.1.3 >Reporter: Marcin L >Priority: Minor > Time Spent: 10m > Remaining Estimate: 0h > > Support for endpoint identification algorithm was added in Java 1.7. > Currently MINA supports providing single SNI name via > org.apache.mina.filter.ssl.SslFilter#PEER_ADDRESS session attribute, but > there is no way verifying it matches the certificate received. > It would be nice if we could provide endpoint identification algorithm to > SslFilter so certificate's common name or subject alternative names are > verified. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
[GitHub] [mina] the-thing opened a new pull request #26: DIRMINA-1122 - added support for endpoint identification algorithm
the-thing opened a new pull request #26: URL: https://github.com/apache/mina/pull/26 https://issues.apache.org/jira/projects/DIRMINA/issues/DIRMINA-1122 We found out that it is not possible to set endpoint identification algorithm while discussing SNI support in quickfixj https://github.com/quickfix-j/quickfixj/issues/276 **Changes** - added support for endpoint identification algorithm to SslFilter - unit tests for SNI CN/SAN certificate matching 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. 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
Re: [mina-sshd] 01/02: [SSHD-660] Add support for serer side openssh host certkeys
Yes, sorry for the noise. I must have used a not clean repo before building or maybe not rebuild from root, not sure. Anyway, I can confirm that it's working for me too. Le lun. 20 avr. 2020 à 15:37, Lyor Goldstein a écrit : > >> I think your forgot to commit the changes to a few files: KeyUtils, > KeyPairProvider > > I don't think so - I have used the cloned repository commits rather that > the PR + the code compiles and passes all tests. > -- Guillaume Nodet
[GitHub] [mina-sshd] lgoldstein commented on issue #122: Allow colon as valid host pattern char to support ipv6 entries in kno…
lgoldstein commented on issue #122: URL: https://github.com/apache/mina-sshd/pull/122#issuecomment-616559748 Please resolve the conflicts and let me know when you feel it is ready to merge 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. 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
Re: [mina-sshd] 01/02: [SSHD-660] Add support for serer side openssh host certkeys
>> I think your forgot to commit the changes to a few files: KeyUtils, KeyPairProvider I don't think so - I have used the cloned repository commits rather that the PR + the code compiles and passes all tests.
[GitHub] [mina-sshd] lgoldstein commented on issue #119: Add support for openssh host key certificates
lgoldstein commented on issue #119: URL: https://github.com/apache/mina-sshd/pull/119#issuecomment-616557210 >> Thanks a lot, but it's not only server side support but also client side (I started with client side actually and had two different commits..). Sorry - too late now - the important thing is that the code is in >> And according to your latest comment in SSHD-895 we should revert the changes in ClientBuilder? I have reviewed the code before merging and have reached the conclusion that I can live with the change you suggested - i.e., include the certified keys by default. I have decided though not to include the `ssh-rsa-sah256` yet since it is still a rather recent feature. 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. 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
[GitHub] [mina-sshd] gnodet opened a new pull request #123: SFTP transfer speeds, clean API
gnodet opened a new pull request #123: URL: https://github.com/apache/mina-sshd/pull/123 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. 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
[GitHub] [mina] snufkinsnorka commented on issue #25: make NioProcessor and NioSocketAcceptor extendable by removing final from class declaration
snufkinsnorka commented on issue #25: URL: https://github.com/apache/mina/pull/25#issuecomment-616402730 We want to extend those in order to add ability of moving IoSession established sockets from processor to processor. We use this when we want to move connections between different servers (which are implemented with different classLoaders) In order to implement this we need for example to override ShadowNioSocketAcceptor.open methods. For example, during open, we no longer need to create the socket, but we do need to register the selector, which is why we also change it to be protected. 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. 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
[GitHub] [mina] elecharny commented on issue #25: make NioProcessor and NioSocketAcceptor extendable by removing final from class declaration
elecharny commented on issue #25: URL: https://github.com/apache/mina/pull/25#issuecomment-616378510 Wondering: why would you make those classes extensible ? 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. 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
[GitHub] [mina] snufkinsnorka opened a new pull request #25: make NioProcessor and NioSocketAcceptor extendable by removing final from class declaration
snufkinsnorka opened a new pull request #25: URL: https://github.com/apache/mina/pull/25 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. 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
