Sioro-Neoku / go-peerflix

Go Peerflix
MIT License
470 stars 119 forks source link

Allow specifying incoming torrent port #27

Closed nochso closed 8 years ago

nochso commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

Sioro-Neoku commented 8 years ago

Looks great, thank you!