Add stdin & stdout to the transfer command #789

townsend2010 · 2019-05-16T20:12:27Z

Fixes #578

This includes running clang-format on all of the changed code.

codecov · 2019-05-20T14:39:53Z

Codecov Report

Merging #789 into master will increase coverage by 0.01%.
The diff coverage is 72.51%.

@@            Coverage Diff            @@
##           master    #789      +/-   ##
=========================================
+ Coverage   68.69%   68.7%   +0.01%     
=========================================
  Files         177     181       +4     
  Lines        6273    6403     +130     
=========================================
+ Hits         4309    4399      +90     
- Misses       1964    2004      +40

Impacted Files	Coverage Δ
src/client/cli/cmd/transfer.h	`100% <ø> (ø)`	⬆️
include/multipass/ssh/sftp_client.h	`0% <0%> (ø)`
src/ssh/sftp_client.cpp	`71.25% <71.25%> (ø)`
src/client/cli/cmd/transfer.cpp	`77.16% <76%> (+2.69%)`	⬆️
src/daemon/daemon_config.cpp	`48.21% <0%> (-1.79%)`	⬇️
src/daemon/daemon.cpp	`28.32% <0%> (-0.07%)`	⬇️
include/multipass/virtual_machine_factory.h	`0% <0%> (ø)`	⬆️
include/multipass/utils.h	`30.76% <0%> (ø)`	⬆️
...tform/backends/qemu/qemu_virtual_machine_factory.h	`0% <0%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbb2e9a...6577076. Read the comment docs.

townsend2010 · 2019-05-20T14:53:51Z

Ok, let's see about getting this in its current form. I plan on switching over to just using this for all types of transfers and remove the SCPClient stuff.

gerboland

This is an excellent feature, thank you. Many thanks for all the additional tests too.

I've only a couple of minor niggles found in the code, which I've reported.

Functionally I've one main concern: I've noticed sometimes files fail to transfer correctly. In my build directory I did:

cat ./bin/text_to_string_array  | ./bin/multipass transfer - x:/home/multipass/text_to_string_array

and then I compared file sizes and md5sums. Sometimes I found the transferred file was different to the original.

Host:

$ ls -l bin/text_to_string_array 
-rwxrwxr-x 1 gerry gerry 137472 May 10 14:23 bin/text_to_string_array

and in VM:

x: $ ls -l text_to_string_array 
-rwxrwxr-x 1 multipass multipass 157010 May 23 11:37 text_to_string_array

I'm trying to identify a pattern, as it only appears to happen with some files that I try. Can you reproduce?

A smaller functional complaint is that if you do something silly like trying to transfer into a directory

cat ./bin/text_to_string_array  | ./bin/multipass transfer - x:/home/multipass                    
transfer failed: [sftp stream] open failed: 'SFTP server: Failure'

the error message isn't great.

Would there be value in checking the destination is valid before trying a transfer? And maybe prompting before possibly overwriting existing files?

But I'm not expecting a perfect UX in this MP. If you can reproduce and resolve the file copy problem I see, that would be good enough for this PR.

include/multipass/ssh/sftp_client.h

src/client/cli/cmd/transfer.cpp

ricab

Thank you for the contribution! I find this adds very worthwhile value and is well coded in general. Very helpful 🙂

There are some non-central things I would like to see improved, but I think this is very close to being in a good state for inclusion. My main concerns are:

mocking stdin/stdout -> relying on C++ api for IO
terminology (e.g. things like template did not make much sense to me)
security: I would recommend getting another careful look focusing on security before merging

include/multipass/ssh/sftp_client.h

tests/test_sftp_client.cpp

src/client/cli/cmd/transfer.cpp

include/multipass/ssh/sftp_client.h

src/client/cli/cmd/transfer.cpp

src/ssh/sftp_client.cpp

townsend2010 · 2019-05-23T17:32:55Z

@gerboland,

I can also reproduce the file size/md5sum issue, What is happening is that when reading from stdin, the code uses a QTextStream to read in the data, assuming it will be actual text from a terminal. However, in your scenario, you are redirecting binary data through cat and then QTextStream is interpreting that data.

I'll have to dig further to figure out whether 1) redirecting binary data is even supported in the sftp protocol, 2) if so, then figure out if cat is at fault or if it's QTextStream, and 3) if it's QTextStream, what a better solution would be.

ricab · 2019-05-23T17:43:02Z

QTextStream

@townsend2010 I believe this would be covered by a move to std::istream, which I suppose would be part of abstracting cin/cout for mocking. See this comment.

- Use the istream/ostream from the Terminal class for stdio. - Clean up the logic around trying to figure out if '-' is used. - Add more tests for streaming to stdout. - Some other code optimizations.

ricab

Very nice, thanks a lot! This is now in very good shape.

I have only some comments/questions and a few very simple requests (and one pending from last review).

include/multipass/ssh/sftp_client.h

src/client/cli/cmd/transfer.cpp

src/ssh/sftp_client.cpp

tests/test_sftp_client.cpp

include/multipass/ssh/sftp_client.h

gerboland · 2019-05-27T14:45:17Z

The transfer bug I noticed is fixed, thanks!

gerboland

Overall looks good! I'm happy to approve this as-is

gerboland · 2019-05-27T14:52:24Z

src/ssh/sftp_client.cpp

+{
+    auto full_destination_path = full_destination(destination_path, mp::utils::filename_for(source_path));
+    SFTPFileUPtr file_handle{
+        sftp_open(sftp.get(), full_destination_path.c_str(), O_WRONLY | O_CREAT | O_TRUNC, file_mode), sftp_close};


My only concern is how easily we overwrite files. Or am I being too nanny-ish?

So did an experiment to see what the actual sftp client does and it will just overwrite the old file with the new file without asking. So I think it's ok to leave as-is, but perhaps if it becomes an issue, we can do something about it in the future.

src/ssh/sftp_client.cpp

ricab

Nice 👍

bors r+

789: Add stdin & stdout to the transfer command r=ricab a=townsend2010 Fixes #578 Co-authored-by: MaximDanilov <[email protected]> Co-authored-by: Chris Townsend <[email protected]>

bors · 2019-05-29T15:00:51Z

Build succeeded

MaksimDanilov and others added 9 commits May 16, 2019 15:32

[client] Make copy-files work with stdin and stdout

9dc9f1e

[client] Refactoring

5d158c0

Add support of sftp protocol for streaming files from stdin

2a2924c

Remove unused header

daabd71

Read fixed packages and send them. Remove cache from client

afbbe5b

add some tests

116c995

add some tests and code review

6d0cb4f

dont need this test

2d1fb3b

transfer: Various fixes needed for rebase on master

68ef049

This includes running clang-format on all of the changed code.

townsend2010 added the no-merge label May 16, 2019

townsend2010 mentioned this pull request May 16, 2019

[client] Make copy-files work with stdin and stdout #639

Closed

townsend2010 changed the title ~~[WiP] Add stdin & stdout to transfer~~ Add stdin & stdout to the transfer command May 20, 2019

townsend2010 removed the no-merge label May 20, 2019

ricab self-requested a review May 22, 2019 16:25

sftp_client: Fix tests and general cleanup

c08d536

townsend2010 force-pushed the add-stdin-stdout-to-transfer branch from beb0adc to c08d536 Compare May 22, 2019 17:14

gerboland suggested changes May 23, 2019

View reviewed changes

include/multipass/ssh/sftp_client.h Outdated Show resolved Hide resolved

src/client/cli/cmd/transfer.cpp Outdated Show resolved Hide resolved

ricab requested changes May 23, 2019

View reviewed changes

sftp_client: Many changes based on review feedback

a7e4d27

- Use the istream/ostream from the Terminal class for stdio. - Clean up the logic around trying to figure out if '-' is used. - Add more tests for streaming to stdout. - Some other code optimizations.

ricab reviewed May 27, 2019

View reviewed changes

gerboland approved these changes May 27, 2019

View reviewed changes

Chris Townsend added 2 commits May 28, 2019 12:33

client/transfer: Few more fixes based on review

e6a8769

tests: Add a few transfer cli tests for stdin/stdout

6577076

ricab approved these changes May 29, 2019

View reviewed changes

bors bot added a commit that referenced this pull request May 29, 2019

Merge #789

6ed1158

789: Add stdin & stdout to the transfer command r=ricab a=townsend2010 Fixes #578 Co-authored-by: MaximDanilov <[email protected]> Co-authored-by: Chris Townsend <[email protected]>

bors bot merged commit 6577076 into master May 29, 2019

bors bot deleted the add-stdin-stdout-to-transfer branch May 29, 2019 15:01

townsend2010 mentioned this pull request May 30, 2019

Try stdio transfer #781

Closed

Add stdin & stdout to the transfer command #789

Add stdin & stdout to the transfer command #789

Uh oh!

Conversation

townsend2010 commented May 16, 2019

Uh oh!

codecov bot commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

townsend2010 commented May 20, 2019

Uh oh!

gerboland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ricab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

townsend2010 commented May 23, 2019

Uh oh!

ricab commented May 23, 2019

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gerboland commented May 27, 2019

Uh oh!

gerboland left a comment

Choose a reason for hiding this comment

Uh oh!

gerboland May 27, 2019

Choose a reason for hiding this comment

Uh oh!

townsend2010 May 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ricab left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented May 29, 2019

Build succeeded

Uh oh!

Uh oh!

codecov bot commented May 20, 2019 •

edited

Loading

ricab left a comment •

edited

Loading