Enabling IPv6 should not disable IPv4

[romanrm]

The option to enable IPv6 should not disable operation on IPv4. Having an IPv6-only BitTorrent client is not useful today, the practical scenario for the foreseeable future is to utilize both protocols simultaneously (dual-stack).

[Chocobo1]

4 spaces indentation

[Chocobo1]

Offtopic, could you also help rewrite line 1457
if (IPs.size() == 0) to if (IPs.isEmpty()) ?

[sledgehammer999]

This was done knowingly this way. At first I had similar code to yours but 2 things changed that:

1. Libtorrent can listen only to 1 interface at a time.
2. If user checks the ipv6 box then he expects the program to listen to ipv6 not ipv4. Qt might return the ipv4 IPs of a network interface before the IPv6 IPs and that way we will connect to ipv4 first and not use ipv6.
3. Some users might have a local IPv6 address on their interface but that address isn't connected to anything(externally) thus resulting in no traffic. That's why ipv6 is explicitly left off if the user hasn't checked the box. Again qt might return the ipv6 addresses first...

In light of all these, I am closing this. If you have some counter argument I'll be happy to hear it.
PS: Thanks for the code though.

[Chocobo1]

@sledgehammer999

Now I think again, the current impl isn't enough to handle IPv6.

1. In IPv6 an interface can have more than 1 address (actually IPv4 can do it too): link.
   qBt interface select seems to assume there is only 1 address per interface. The correct should show a list of pair like <eth0, (IPv4) 15.15.15.15>, <eth0, (IPv6) 2004::>.
2. IPv6 should be a opt-in choice, like this PR does, shouldn't be only IPv6.
3. The issue you mentioned can be avoided by rearranging the list, that is, putting global-scope address at start of the list and link-local address at end of the list. Like this:
   IPv6 (non link-local) -> IPv4 (non link-local) -> IPv6 (link-local) -> IPv4 (link-local) -> IPv6 (loopback) -> IPv4 (loopback)
   (IPv6 entries will be empty when "Enable IPv6" option is not selected)

I could try to rewrite it, any other consideration?

[sledgehammer999]

@Chocobo1 read the code more carefully. qbt doesn't assume ONE address is returned. Inside Session::setListeningPort() it will try all the addresses until error_code isn't set.

As I said, libtorrent listens only to one address at a time. If you set another it will stop listening to the previous one. So if the user has checked "Listen to IPv6 address" he really wants to listen to IPv6 only. Otherwise he would leave it empty.

This PR would make sense if libtorrent indeed let us listen to multiple addresses.

[Chocobo1]

@Chocobo1 read the code more carefully. qbt doesn't assume ONE address is returned. Inside Session::setListeningPort() it will try all the addresses until error_code isn't set.

Yes, the try-until-success logic in Session::setListeningPort() is right. My previous post point 3 is trying to minimize the chance of listening on the wrong interface.

When I said Now I think again, the current impl isn't enough to handle IPv6. I mean the interface selection in Advanced Options doesn't let user select the address they want.

For example: eth0 has 2 valid IPv6 address 2001::1 & 2002::1. When Network interface option is set to eth0, current code will blindly select one address of eth0 to listen on because we only let user select interface not address.
IMO, the correct UI should let user see what addresses is binded to the interface, i.e., the user should have 2 (address) choices instead of 1 (interface) choice and the user can select only 1 address to listen on.

So if the user has checked "Listen to IPv6 address" he really wants to listen to IPv6 only. Otherwise he would leave it empty.

Well, I'm still not convinced by this idea. Most modern impl of IPv6 socket can also listen to IPv4 connection, see IPV6_V6ONLY option in http://man7.org/linux/man-pages/man7/ipv6.7.html
Basically, IPv6 socket with that option set to off should cover most use case.

I'm not going to argue anymore, but if you know bittorrent protocol works otherwise (haven't read all BEPs), I am all ears.

[sledgehammer999]

When I said Now I think again, the current impl isn't enough to handle IPv6. I mean the interface selection in Advanced Options doesn't let user select the address they want.

For example: eth0 has 2 valid IPv6 address 2001::1 & 2002::1. When Network interface option is set to eth0, current code will blindly select one address of eth0 to listen on because we only let user select interface not address.

I misunderstood you. Yes, the above seems reasonable.

I'm not going to argue anymore, but if you know bittorrent protocol works otherwise (haven't read all BEPs), I am all ears.

This has nothing to do with the bittorrent protocol. It is libtorrent that is limiting us. libtorrent::session::listen_on() takes only one address. If you call it multiple times, it stops listening to the previous address.

[keith2015]

Unsubscribe me

Sent from Yahoo Mail on Android

On Tue, Dec 29, 2015 at 6:07 AM, romanrmnotifications@github.com wrote:
The option to enable IPv6 should not disable operation on IPv4. Having an IPv6-only BitTorrent client is not useful today, the practical scenario for the foreseeable future is to utilize both protocols simultaneously (dual-stack).

You can view, comment on, or merge this pull request online at:

  #4469

Commit Summary

• Enabling IPv6 should not disable IPv4

File Changes

• M src/base/bittorrent/session.cpp (5)

Patch Links:

• https://github.com/qbittorrent/qBittorrent/pull/4469.patch
• https://github.com/qbittorrent/qBittorrent/pull/4469.diff

—
Reply to this email directly or view it on GitHub.

[sledgehammer999]

@keith2015 you have to unsubscribe yourself. We can't. Come to this link and hit "unsubscribe" from the sidepanel.