Rework the listening IP/interface selection code

[sledgehammer999]

Closes #11561

It started as a simple fix for #11561. But as I dug deeper I fixed more and more things. And it became a full blown refactor.

With this PR:

1. As per 1.2.x docs, the default setting (aka all interfaces + all addresses) is to also explicitly set the IPv6_ANY address
2. Now if a user chooses "all addresses" from a specific interface, we tell libtorrent to bind to that specific interface instead of listing to it all the addresses of the interface. This has the benefit that libtorrent will automatically follow any changes to that interface. This is of benefit to VPN users when their connection drops and then they get assigned a new "local" ip.
3. Two new options in the drop down menu for addresses. All IPv4 Addresses and All IPv6 Addresses.
4. Due to the above, the option "Listen to IPv6 address" is removed. This warrants a WebAPI major version bump (not done in this PR).
5. I discovered a Qt bug concerning the textual representation of IPv6 and scope id. Read the code comments for details. This bug essentially resulted in libtorrent not binding on the set IPv6 address but instead to 0.0.0.0.

PS: I am not very good with choosing variable/function names.

[Chocobo1]

Some quick reviews.. no need to wait for my review completion if you want to force push/update.

[sledgehammer999]

Guys, I think I addressed all your comments so far. Take a look at the new commit.

[glassez]

canonicalIPv6Addr shouldn't return non-IPv6 address!

[sledgehammer999]

So return a null QHostAddress?

[sledgehammer999]

In this push, I addressed almost all the comments, apart 2 from glassez.

[Chocobo1]

No more comments from me.

Also I noticed this patch: arvidn/libtorrent#4166
If that patch could simplify our work here, it might be worth to add some #if conditionals to the relevant code.

[glassez]

If applying this function to IPv4 address isn't valid "use case" you should prevent it in any way (at least add an appropriate assertion). Otherwise it may allow logically incorrect code.

[sledgehammer999]

I am confused. How do I prevent it in any way?
And won't the assertion work only in debug?

[glassez]

How do I prevent it in any way?

You can throw RuntimeError...

Or you can modify it to be valid for any IP (and rename it to canonicalIPAddr).

[glassez]

Or even it can return IPv6 representstion of IPv4... unless I am wrong it is possible.

[sledgehammer999]

QHostAddress::toIPv6Address() seems to do the job. So it becomes:

if (addr.protocol() != QAbstractSocket::IPv6Protocol)
                return {addr.toIPv6Address()};

[glassez]

And also apply all remaining modifications.

[glassez]

Or it already in "canonical" form?

[sledgehammer999]

Or it already in "canonical" form?

It can't be, because at this point it is IPv4.
If you mean below, there is no way to know beforehand. So we convert unconditionally.

[glassez]

I would implement another variant instead. I mean canonicalIPAddr that returns IP of the same type as its argument.
BTW, we can use it unconditionally since it always returns valid result.

[sledgehammer999]

I think I addressed everything and I did some furhter refactor. Due to that I implemented it as a separate temporary commit. It will be easier for you to see what I changed.

[sledgehammer999]

The missing part is the webui code itself. @Chocobo1 will you be able to plug in the correct js/html changes?

[Chocobo1]

@Chocobo1 will you be able to plug in the correct js/html changes?

Currently I'm low on free time to write code unfortunately.

[glassez]

An empty string for all address is fine for me or even better IMO: 0.0.0.0,::

Isn't empty string treated as "any IP" by app core? Then it should be used here as well.

[sledgehammer999]

Now the webui receives an empty string to mean all addresses, 0.0.0.0 to mean All IPv4 addresses and :: to mean All IPv6 addresses. This seems to have the benefit of the least possible code/refactoring.

I also changed how the GUI populates the comboboxes to match the logic of the WebUI comboboxes.

And I probably changed other things here and there. So forgive me but please take another look at the whole thing.

[Chocobo1]

only some minor things left, overall it's looking good.

[sledgehammer999]

On another note, I just noticed that we do a lot of

// ifaceAddr is of type QHostAddres
functionThatTakesQString(ifaceAddr.isNull() ? QString {} : ifaceAddr.toString());

But I realize that we can collapse it to:

functionThatTakesQString(ifaceAddr.toString());

I'll make the changes later (only code affected by me).

[sledgehammer999]

Closes #11632

[sledgehammer999]

@glassez do you approve?

[glassez]

I would still change canonicalIPv6Addr to suggested canonicalIPAddr form. But you can ignore this opinion.

[sledgehammer999]

I would still change canonicalIPv6Addr to suggested canonicalIPAddr form.

IMO, I don't want to give the impression that the function can be used with IPv4. (Yes, it will work but it shouldn't be used for IPv4).

[glassez]

it shouldn't be used for IPv4

You cannot forbid it since function accepts generic host address (function name isn't enough, there can be use case when parameter is bypassed unchecked). You either can break it at runtime (throw an exception) or change it to be valid (have a meaning) for any kind of host address.
If my argument is irrelevant to you, you can merge this PR, I have nothing to add.

[sledgehammer999]

No, I meant that it is discouraged through the function name.

Thank you all for the reviewing.

[ghost]

This introduced the tracker not working bug in v 4.2.1
The annnounce goes through and tracker status says "working" but still the torrent gets moved to error section unless you expilcity set "Optional IP address to bind to" to "All IPv4 Addresses"
#11691
@sledgehammer999 @glassez @Chocobo1

[FranciscoPombal]

@sledgehammer999 @glassez @Chocobo1
There is new information available about this topic in the libtorrent documentation:
arvidn/libtorrent#4267

Not sure if this warrants any changes/additions to the code introduced in this PR, but I thought I should give the heads-up.