Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable sharing the binary file and use a larger buffer #2786

Merged
merged 14 commits into from
Aug 16, 2022

Conversation

qidu
Copy link
Contributor

@qidu qidu commented Aug 1, 2022

Description

Update an examle for trasnfering a binary file

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
    I have added tests that prove my fix is effective or that my feature works
    A changelog entry has been made in the appropriate crates

@qidu qidu marked this pull request as draft August 1, 2022 05:52
@qidu qidu marked this pull request as ready for review August 1, 2022 05:53
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request.

I am in favor of the move to Vec<u8>. I suggest we don't write to a file but continue to write to stdout.

@@ -196,6 +208,8 @@ enum CliArgument {
name: String,
},
Get {
#[clap(long)]
path: PathBuf,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of providing a file path for the retrieved file to be written to, how about leveraging unix pipelines?

E.g. one would do:

cargo run -- get [...] > my_file

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Thanks for your advices. I've changed the output to stdout().

Comment on lines 139 to 148
let mut reader = BufReader::new(File::open(&path)?);
let mut vec = Vec::<u8>::new();
let mut buffer = reader.fill_buf()?;
let mut len = buffer.len();
while len > 0 {
vec.extend_from_slice(buffer);
reader.consume(len);
buffer = reader.fill_buf()?;
len = buffer.len();
}
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is reading the entire file into vec, what is the difference to the previous version, i.e. std::fs::read_to_string? If you want to read an entire file as u8, why not use the below?

https://doc.rust-lang.org/std/fs/fn.read.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. fixed.

@@ -134,8 +135,7 @@ async fn main() -> Result<(), Box<dyn Error>> {
// Reply with the content of the file on incoming requests.
Some(network::Event::InboundRequest { request, channel }) => {
if request == name {
let file_content = std::fs::read_to_string(&path)?;
network_client.respond_file(file_content, channel).await;
network_client.respond_file(&std::fs::read(&path)?, channel).await;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
network_client.respond_file(&std::fs::read(&path)?, channel).await;
network_client.respond_file(std::fs::read(&path)?, channel).await;

Why borrow here when we clone again later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but expected &Vec<u8> here. any better choice?

Co-authored-by: Max Inden <mail@max-inden.de>
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM

One small suggestion.

Comment on lines 354 to 356
pub async fn respond_file(&mut self, file: &Vec<u8>, channel: ResponseChannel<FileResponse>) {
self.sender
.send(Command::RespondFile { file, channel })
.send(Command::RespondFile { file: file.to_vec(), channel })
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think the reference on line 354 is unnecessary if we are going to create a copy anyway and it will remove the diff on line 356.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done. Thanks~

examples/file-sharing.rs Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the follow ups.

@qidu
Copy link
Contributor Author

qidu commented Aug 12, 2022

Thanks~

@mxinden mxinden merged commit 0e5a25d into libp2p:master Aug 16, 2022
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.

None yet

3 participants