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

Allow specifying incoming torrent port #27

Merged
merged 3 commits into from
Jul 18, 2016

Conversation

nochso
Copy link
Contributor

@nochso nochso commented Mar 7, 2016

Adds a new flag -torrent-port to specify the listening port for torrent connections.

However having both port and torrent-port might be confusing. How about http-port and torrent-port or http and torrent?

As of now:

Usage of ./go-peerflix:
  -player string
        Open the stream with a video player (VLC, MPV, MPlayer)
  -port int
        Port to stream the video on (default 8080)
  -seed
        Seed after finished downloading
  -tcp
        Allow connections via TCP (default true)
  -torrent-port int
        Port to listen for incoming torrent connections (default 6882)

@Sioro-Neoku
Copy link
Owner

I feel a bit reluctant, as I'm not sure many people need this (and the client.NewClient() method is just getting increasingly bloated).

One thing I'd definitely change is using the default torrent port (6882), as for many users this port is probably blocked (or monitored?).
Many clients randomize their torrent port. If we'd do the same, it is a feature I'm happy to merge.

@nochso
Copy link
Contributor Author

nochso commented Mar 13, 2016

I agree on randomizing by default / if left empty. Probably in the range of 1025-10000 to avoid <=1024 root related issues.

Otherwise, to me it makes sense. You're most likely uploading anyway, so why not help other peers which are NATed or otherwise firewalled? In my experience download speeds increase too.

Not sure what to do about client.NewClient(). Maybe set the listening port after instantiation?

@Sioro-Neoku
Copy link
Owner

An idiomatic go practice is to replace a long list of arguments with a config structure.

Something like

type ClientConfig struct {
  torrentPath string
  port int
  torrentPort int
  seed bool
  tcp bool
}

A method on the structure could fill in the default values.

@nochso
Copy link
Contributor Author

nochso commented Jul 15, 2016

Turns out port randomization has its drawbacks: http://dev.deluge-torrent.org/ticket/2133

How about keeping the current default by anacrolix/torrent and allowing to override it? Alternatively, one could generate a random port based on something that is host/machine specific and doesn't change.

@nochso
Copy link
Contributor Author

nochso commented Jul 15, 2016

Ok, I've added ClientConfig with a method for instantiating with defaults.

The flags are bound to the config which is then passed to the client after flag parsing.

@Sioro-Neoku
Copy link
Owner

Looks pretty good now!

One thing I'd change is to make the fields of the ClientConfig structure public:

type ClientConfig struct {
    TorrentPath    string
    Port           int
    TorrentPort    int
    Seed           bool
    Tcp            bool
    MaxConnections int
}

I understand that you just copied what I wrote, so it's clearly on me =)

@nochso
Copy link
Contributor Author

nochso commented Jul 16, 2016

Ok :) I've amended the last commit and made the fields public.

@Sioro-Neoku
Copy link
Owner

Looks great, thank you!

@Sioro-Neoku Sioro-Neoku merged commit abb60b4 into Sioro-Neoku:master Jul 18, 2016
@nochso nochso deleted the torrent-port branch January 20, 2017 19:31
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