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

Basic IPv6 support for Jamulus #1455

Closed
wants to merge 7 commits into from

Conversation

mstolle629
Copy link

in our region, there are many people on ISP's with only "dual stack lite" ipv4 support (Vodafone, 1&1), which have problems with ipv4, because they use a central NAT instance.

This patch introduces ipv6 support.

It adds command line options "-4" and "-6", which decide, in which sequence NetworkAddresses are parsed. It also includes autosensing, if there is no ipv6 support in the kernel or driver - in that case it falls back to ipv4 and the old code.

In addition, it uses recvmsg() and sendmsg(), so that it can read the IP address, on which packets are received. In Case of Windows, it uses WSASendMsg and WSARecvMsg.

Tested on Windows, Linux and Mac.
In our environment, the response times of ipv6 are about 6ms faster than the response times of ipv4, because the connection on the internet is more direct, but this may vary.

ipv6 support
socket.cpp ipv6 support for Linux/Windows/Mac
socket.h extension for Windows Pointers to WSASendMsg and WSARecvMsg
added autosensing, if there is no ipv6 stack available in OS, fall back to ipv4 communication
works with mac os, windows  and linux
@gilgongo gilgongo linked an issue Apr 6, 2021 that may be closed by this pull request
@gilgongo
Copy link
Member

gilgongo commented Apr 6, 2021

See also #1017

for compatibility with elder mac
@mstolle629
Copy link
Author

It is essential for the choir i am singing in and for friends to have ipv6 support working, because in our regions (near Frankfurt and Cologne) many people use internet providers which don't have native ipv4 support, but have "Dual stack lite" with ipv6 native and ipv4 only over a central NAT instance, which leads to problems with ipv4.

I have to keep backward compatiblity for jamulus and not disturb the registration on central server, therefore i decided to introduce a new option "ipmode", which defaults to ipv4 first and ipv6, if there is no ipv4 address for the server.
I introduced two command line option flags "-4" and "-6".
"-4 -6" = default
"-6 -4" = -v6 first, -4 (if there is no ipv6 support for the Hostname)
"-4" = ipv4 only
"-6" = ipv6 only

For backward compatiblity, the connection to the central registration server is done by ipv4 only and transparent.

I think, we should discuss, whether a registration for ipv6 on central server should be supported. My idea would be, to register the server twice, one time for ipv4 (as usual) and a second time with a Server name Postfix of "_v6" for ipv6.

I patched ParseNetworkAddress(), so that it supports the additional parameter of "ipmode" with default to "0", so that old sources with ipv4 only are supported.

I replaced sendto() and recvfrom() with sendmsg() and recvmsg(), added some setsockopts() at the start of a socket(), so that i can read the local ip address, which the client connected to, on the server, to choose the right ip address for the answer - i think, this is useful for ipv4 only, too. Therefore I added a field "LocalAddress" to Hostaddr. In addition, this code could be easily extended for supporting setting TOS/DSCP bits in the messages sent, which sometimes could be good (most times these flags are resetted by the ISP).

I used "ipv4 mapped ipv6". In case, if there is no support for v6 in the kernel, the code falls back to its origin code of recvfrom and sendto with native v4 code.

There is another implementation for ipv6 support in #1017, but this is my own one and works for me.
I would be glad, if i can inspire and help someone with my work - and if someone could help me and review it.

It works on Windows, Mac and Linux.

@djfun
Copy link
Contributor

djfun commented Apr 8, 2021

For the command line parameters, I would prefer a solution that is not dependent on the order

@pljones
Copy link
Collaborator

pljones commented Apr 10, 2021

For the command line parameters, I would prefer a solution that is not dependent on the order

Agreed. If I've understood what's being said above, something like this?

"-4 --ip46" = default, IPv4 first, IPv6 if there's no native IPV4 address
"-6 --ip64" = IPv6 first, IPv4 if there is no native IPv6 address
"   --ip4only" = IPv4 only (refuse to connect to any IPv6 addresses)
"   --ip6only" = IPv6 only (refuse to connect to any IPv4 addresses)

@@ -0,0 +1,607 @@
/******************************************************************************\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has this file been added outside src?

@@ -31,7 +31,8 @@ CClient::CClient ( const quint16 iPortNumber,
const QString& strMIDISetup,
const bool bNoAutoJackConnect,
const QString& strNClientName,
const bool bNMuteMeInPersonalMix ) :
const bool bNMuteMeInPersonalMix,
int ipmode ) :
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use spaces, be consistent with the existing source.

@@ -62,6 +63,7 @@ CClient::CClient ( const quint16 iPortNumber,
bEnableOPUS64 ( false ),
bJitterBufferOK ( true ),
bNuteMeInPersonalMix ( bNMuteMeInPersonalMix ),
iipmode ( ipmode ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above - spaces not tabs, please.

@@ -389,7 +391,8 @@ bool CClient::SetServerAddr ( QString strNAddr )
{
CHostAddress HostAddress;
if ( NetworkUtil().ParseNetworkAddress ( strNAddr,
HostAddress ) )
HostAddress,
iipmode ) )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@@ -113,7 +113,8 @@ class CClient : public QObject
const QString& strMIDISetup,
const bool bNoAutoJackConnect,
const QString& strNClientName,
const bool bNMuteMeInPersonalMix );
const bool bNMuteMeInPersonalMix,
int ipmode=0 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@@ -355,6 +356,7 @@ class CClient : public QObject

bool bJitterBufferOK;
bool bNuteMeInPersonalMix;
int iipmode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@@ -88,6 +88,7 @@ int main ( int argc, char** argv )
QString strServerListFilter = "";
QString strWelcomeMessage = "";
QString strClientName = "";
int ipmode = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@@ -140,6 +141,41 @@ int main ( int argc, char** argv )
continue;
}

// Use ipv4 protocol
if ( GetFlagArgument ( argv,
i,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@@ -846,6 +883,8 @@ QString UsageArguments ( char **argv )
" -p, --port set your local port number\n"
" -t, --notranslation disable translation (use English language)\n"
" -v, --version output version information and exit\n"
" -4, --ipv4 use IPv4 Protocol\n"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

@@ -1097,7 +1102,9 @@ class NetworkUtil
{
public:
static bool ParseNetworkAddress ( QString strAddress,
CHostAddress& HostAddress );
CHostAddress& HostAddress,
int inetmode=0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow standard alignment of code.


CHostAddress ( const CHostAddress& NHAddr ) :
InetAddr ( NHAddr.InetAddr ),
iPort ( NHAddr.iPort ) {}
iPort ( NHAddr.iPort ),
LocalAddr ( static_cast<quint32> ( 0 )) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow standard alignment of code - even where that means adjusting existing code.

@@ -768,16 +768,19 @@ class CHostAddress

CHostAddress() :
InetAddr ( static_cast<quint32> ( 0 ) ),
iPort ( 0 ) {}
iPort ( 0 ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs reformatting so all the initialisers align.

@mstolle629
Copy link
Author

mstolle629 commented Apr 10, 2021 via email

@mstolle629
Copy link
Author

mstolle629 commented Apr 10, 2021 via email

@mstolle629
Copy link
Author

mstolle629 commented Apr 10, 2021 via email

@mstolle629
Copy link
Author

mstolle629 commented Apr 10, 2021 via email

@pljones
Copy link
Collaborator

pljones commented Apr 12, 2021

Hello Peter Jones, thanks for your review and the indent patches of my code. Since i am new in your Jamulus forum, i don't know what to do. I would like to follow your work and patch the source. Shall i patch my source and upload it again? Do i have to generate another pull request? I don't think i can accept your changes directly. I think, i have to patch my source, remove the socket.cpp outside the src directory and upload the patched source again. Is this correct? Thanks again and in advance, mstolle629

The process is:

  • get a github account
  • fork the original code to your own account
  • clone your forked code to your local machine for development
  • create a branch on your local machine for making changes
  • make your changes locally
  • commit changes to your branch and push to your github fork regularly
  • keep your code up to date
git checkout master ;# switch to your master branch
git pull upstream master ;# to sync with the original code
git push ;# to update your fork
git checkout - ;# switch back to your working branch
git rebase master ;# to rebase onto the updated code
# if there are conflicts, you need to repair them _carefully_
# if there are no conflicts, you need to review the diff to master _carefully_ to make sure it's correct still
git push --force ;# to update your branch on your fork
  • when you're ready for review, raise the pull request from your branch on github to the original code
  • if you need to make changes
    • make sure your code is up to date, as above
    • make your changes locally
    • commit changes to your branch and push to your github fork
    • that will automatically update the pull request

@leoguiders
Copy link

For the command line parameters, I would prefer a solution that is not dependent on the order

Agreed. If I've understood what's being said above, something like this?

"-4 --ip46" = default, IPv4 first, IPv6 if there's no native IPV4 address
"-6 --ip64" = IPv6 first, IPv4 if there is no native IPv6 address
"   --ip4only" = IPv4 only (refuse to connect to any IPv6 addresses)
"   --ip6only" = IPv6 only (refuse to connect to any IPv4 addresses)

--ip46 and --ip64 don't seem very self-explanatory, maybe --preferip4 and --preferip6 would be easier to understand by just looking at parameters?

@softins
Copy link
Member

softins commented Apr 22, 2021

For the command line parameters, I would prefer a solution that is not dependent on the order

Agreed. If I've understood what's being said above, something like this?

"-4 --ip46" = default, IPv4 first, IPv6 if there's no native IPV4 address
"-6 --ip64" = IPv6 first, IPv4 if there is no native IPv6 address
"   --ip4only" = IPv4 only (refuse to connect to any IPv6 addresses)
"   --ip6only" = IPv6 only (refuse to connect to any IPv4 addresses)

--ip46 and --ip64 don't seem very self-explanatory, maybe --preferip4 and --preferip6 would be easier to understand by just looking at parameters?

I would keep the prefix --ip for consistency, and so use --ip4pref and --ip6pref.

@Hk1020 Hk1020 mentioned this pull request Apr 25, 2021
@softins
Copy link
Member

softins commented Aug 10, 2021

Closing this in favour of #1938 instead.

@softins softins closed this Aug 10, 2021
@pljones pljones removed a link to an issue Feb 19, 2022
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.

6 participants