You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2021/01/13 10:31:51 UTC
[GitHub] [mina-sshd] chrjoedal opened a new pull request #180: Added support for sftp client posix rename
chrjoedal opened a new pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180
The read-me file for SFTP states, that posix-rename is supported, but the posix rename extension for the sftp client was newer made.
I have made the small implementation, supporting the posix-rename command for the sftp client.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-759457596
> > @chrjoedal looks good. Is there any way you could add a test for the new extension ?
>
> I have tried to give it a look.
> Problem is, that I have only created the client-side support of posix-rename
> I have not looked into enabling the support for posix-rename server-side.
> When I then try to make the unit-test for posix-rename, I get the exception:
> `java.lang.AssertionError: Extension not supported: posix-rename@openssh.com`
> It is therefore a bit hard to test, but I am open for suggestions, as this is my first code change to mina-sshd, so there are many elements in the code, I have not covered.
>
> I have made the test with my local sftp server (for which I needed the feature), where it works fine.
Ah, the best way would be to add the server-side too. I don't think it is much more complicated than the server side, the only thing is that we'll need to actual _rename_ implementation, but that should not be very difficult. Let me know if you need some help.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] chrjoedal closed pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
chrjoedal closed pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] chrjoedal commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
chrjoedal commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-759440306
> @chrjoedal looks good. Is there any way you could add a test for the new extension ?
I have tried to give it a look.
Problem is, that I have only created the client-side support of posix-rename
I have not looked into enabling the support for posix-rename server-side.
When I then try to make the unit-test for posix-rename, I get the exception:
`java.lang.AssertionError: Extension not supported: posix-rename@openssh.com`
It is therefore a bit hard to test, but I am open for suggestions, as this is my first code change to mina-sshd, so there are many elements in the code, I have not covered.
I have made the test with my local sftp server (for which I needed the feature), where it works fine.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#discussion_r556682056
##########
File path: sshd-sftp/src/main/java/org/apache/sshd/sftp/client/extensions/openssh/helpers/OpenSSHPosixRenameExtensionImpl.java
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.sftp.client.extensions.openssh.helpers;
+
+import java.io.IOException;
+import java.util.Map;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.sftp.client.RawSftpClient;
+import org.apache.sshd.sftp.client.SftpClient;
+import org.apache.sshd.sftp.client.extensions.helpers.AbstractSftpClientExtension;
+import org.apache.sshd.sftp.client.extensions.openssh.OpenSSHPosixRenameExtension;
+import org.apache.sshd.sftp.common.extensions.openssh.PosixRenameExtensionParser;
+
+/**
+ * @author <a href="mailto:chr.joedal@gmail.com">Christian Schou Jødal</a>
+ */
+public class OpenSSHPosixRenameExtensionImpl extends AbstractSftpClientExtension implements OpenSSHPosixRenameExtension {
+ public OpenSSHPosixRenameExtensionImpl(SftpClient client, RawSftpClient raw, Map<String, byte[]> extensions) {
+ super(PosixRenameExtensionParser.NAME, client, raw, extensions);
+ }
+
+ @Override
+ public void posixRename(String oldPath, String newPath) throws IOException {
+ Buffer buffer = getCommandBuffer(Integer.BYTES + ((CharSequence) oldPath).length() + ((CharSequence) newPath).length());
Review comment:
Why is the cast of `String` to `CharSequence` here ? `String` already implements `CharSequence` - simply call `oldPath.length()/newPath.length()` directly without any cast
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] chrjoedal commented on a change in pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
chrjoedal commented on a change in pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#discussion_r557082983
##########
File path: sshd-sftp/src/main/java/org/apache/sshd/sftp/client/extensions/openssh/helpers/OpenSSHPosixRenameExtensionImpl.java
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.sftp.client.extensions.openssh.helpers;
+
+import java.io.IOException;
+import java.util.Map;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.sftp.client.RawSftpClient;
+import org.apache.sshd.sftp.client.SftpClient;
+import org.apache.sshd.sftp.client.extensions.helpers.AbstractSftpClientExtension;
+import org.apache.sshd.sftp.client.extensions.openssh.OpenSSHPosixRenameExtension;
+import org.apache.sshd.sftp.common.extensions.openssh.PosixRenameExtensionParser;
+
+/**
+ * @author <a href="mailto:chr.joedal@gmail.com">Christian Schou Jødal</a>
+ */
+public class OpenSSHPosixRenameExtensionImpl extends AbstractSftpClientExtension implements OpenSSHPosixRenameExtension {
+ public OpenSSHPosixRenameExtensionImpl(SftpClient client, RawSftpClient raw, Map<String, byte[]> extensions) {
+ super(PosixRenameExtensionParser.NAME, client, raw, extensions);
+ }
+
+ @Override
+ public void posixRename(String oldPath, String newPath) throws IOException {
+ Buffer buffer = getCommandBuffer(Integer.BYTES + ((CharSequence) oldPath).length() + ((CharSequence) newPath).length());
Review comment:
You are correct - I have made a push, fixing it.
It was a copy-paste error from an other place in the code.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-760323872
Merged + acknowledgement as [498bf10e58d47d2899fe42c3dda4074b2ec63ca8](https://github.com/apache/mina-sshd/commit/498bf10e58d47d2899fe42c3dda4074b2ec63ca8) - many thanks for the contribution. You can close the PR.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-759586401
>> Ah, the best way would be to add the server-side too.
I agree that it would be best, but the PR has value without it as well. @gnodet Unless there is a strong objection I am willing to merge this PR once the minor fix I pointed out is done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-759587506
> > > Ah, the best way would be to add the server-side too.
>
> I agree that it would be best, but the PR has value without it as well. @gnodet Unless there is a strong objection I am willing to merge this PR once the minor fix I pointed out is done.
Agreed, no problem to merge the client side.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] gnodet commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
gnodet commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-759408326
@chrjoedal looks good. Is there any way you could add a test for the new extension ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] chrjoedal commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
chrjoedal commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-759974730
I made the minor fix suggested, and have made a new push.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] chrjoedal commented on pull request #180: Added support for sftp client posix rename
Posted by GitBox <gi...@apache.org>.
chrjoedal commented on pull request #180:
URL: https://github.com/apache/mina-sshd/pull/180#issuecomment-759474220
> > > @chrjoedal looks good. Is there any way you could add a test for the new extension ?
> >
> >
> > I have tried to give it a look.
> > Problem is, that I have only created the client-side support of posix-rename
> > I have not looked into enabling the support for posix-rename server-side.
> > When I then try to make the unit-test for posix-rename, I get the exception:
> > `java.lang.AssertionError: Extension not supported: posix-rename@openssh.com`
> > It is therefore a bit hard to test, but I am open for suggestions, as this is my first code change to mina-sshd, so there are many elements in the code, I have not covered.
> > I have made the test with my local sftp server (for which I needed the feature), where it works fine.
>
> Ah, the best way would be to add the server-side too. I don't think it is much more complicated than the server side, the only thing is that we'll need to actual _rename_ implementation, but that should not be very difficult. Let me know if you need some help.
I have never used the server part of this module - only the client path. So I do not think I am the right for the job, as I have newer used it, started it, or even looked in the code.
I just made this little extension to the client, and submitted it, so others could benefit from it as well :)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org