-
Notifications
You must be signed in to change notification settings - Fork 13
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
Added client to rs-tftpd #22
Conversation
From an overview this looks great! I liked how you used the feature flags so there’s almost no code overhead. I see that you’ve opened this as a draft, but it looks ready for review. Do you want me to review this? |
I asked some people from my work to take a look, but If you want to review I can remove the draft. |
Yeah no worries, I will check it when it’s ready to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. just some small improvements in the log ouput.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Everything overall looks great, I have just some comments about the code style and some copy-paste errors.
That should be all of your requested changes. |
LGTM! |
|
||
To connect the client to a tftp server running on IP address `127.0.0.1`, read-only, on port `1234` and download a file named `example.file` | ||
```bash | ||
tftpd client example.file -i 0.0.0.0 -p 1234 -d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would have been cleaner to just add a new binary to the Cargo.toml
[[bin]]
name = "tftpc"
path = "src/client_main.rs"
required-features = ["client"]
[[bin]]
name = "tftpd"
path = "src/main.rs"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like to open a new PR for this? We can merge it in before doing a new release.
Implements #21
Add a TFTP client, enabled with the client feature: Example:
cargo build --features client
The client supports TFTP Option Extension as defined by RFC 2347
A user can set the following options:
and the client automatically sets the Transfer Size Option RFC 2349
Client CLI options:
Server CLI options (ONLY WHEN CLIENT FEATURE IS ENABLED):