Skip to content

Conversation

@jonathanmorley
Copy link

No description provided.

Copy link
Owner

@iddm iddm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this PR heads towards a good goal, it needs some changes before merging.


let send_file = |sess: &Session, local_path: &Path, remote_path: &Path| {
let mut buffer = vec![0; *PAGE_SIZE as usize];
let mut buffer = vec![];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used PAGE_SIZE especially because it is good for performance to read memory in size multiple to page size. That is why I used it. Windows has also page size parameter, and almost every OS has it actually. It is also possible to read it in windows. As for the rust language, the function to take this constant is probably is defined either in libc crate or winapi crate, I don't know. What's more important, removing the constant as you did here simply will not work: we must allocate some memory to read into later in the line:

while let Ok(read_bytes) = file.read(&mut buffer) {

As we did not allocate anything in the vector above, our vector size is zero and so we will try to read 0 bytes from the file and it will last forever. You may try doing this in a simple example:

fn main() {
    use std::io::Read;

    let mut f = std::fs::File::open("r.rs").unwrap();
    let mut buf = vec![];

    while let Ok(read_bytes) = f.read(&mut buf) {
        println!("Read bytes len: {}", read_bytes);
    }
}

A good PR would be to get PAGE_SIZE in windows as well as we did for unix. Could you do it please?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iddm iddm changed the title Windows Add windows os support Oct 29, 2018
@iddm iddm mentioned this pull request Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants