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

Specify local interface for TFTP sockets #17

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vorot
Copy link

@vorot vorot commented Aug 7, 2019

Sometimes, UDP packets can be routed via a wrong (especially wireless) interface causing problems in TFTP protocol.
In order to prevent that, an option to specify the network interface is added.

@Callisto82
Copy link
Owner

Hi! I like most of the pull request. However, why don't you provide the local address as an optional parameter in the TftpClient constructor? That's also where the remote address is supplied, so it would kind of make sense, wouldn't it?

@vorot
Copy link
Author

vorot commented Sep 2, 2019

Hi! First of all, thank you for the library.

However, why don't you provide the local address as an optional parameter in the TftpClient constructor?

There are several reasons

  • [main] The way I modified your library was the minimal change to fulfill my needs. I do not like to introduce new object members to simply pass a parameter.
    • The usage pattern of TftpClient assumes constructing the client object and creating the transfer object close to each other, which makes it not really important how to pass addresses.
    • If you wish, you may move this parameter to the constructor.
  • It is a nightmare to support the code that uses optional parameters.
  • Theoretically, I can imagine the situation when one client sends different files via different interfaces. I do not think it is going to be used by anybody, but one of my prototypes first selected where to send a file and only after that enumerated network interfaces to select the right one.

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