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

CNetwork - implement IPv6 #7030

Closed
wants to merge 42 commits into
base: master
from

Conversation

Projects
None yet
@mk01
Contributor

mk01 commented Apr 29, 2015

rewrite of network class(es) for abstracted universal ipv6/v4 support

Show outdated Hide outdated xbmc/network/Network.cpp Outdated
@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Apr 30, 2015

Member

I'd like to consider this for Isengard. Objections?

Member

FernetMenta commented Apr 30, 2015

I'd like to consider this for Isengard. Objections?

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 Apr 30, 2015

Member

I don't mind

Member

Jalle19 commented Apr 30, 2015

I don't mind

@Paxxi

This comment has been minimized.

Show comment
Hide comment
@Paxxi

Paxxi Apr 30, 2015

Member

I've only gone over this quickly but I'm a bit concerned about how it affects CURL::Parse(we try to find port number by searching for : from the start of the string) and URIUtils::IsHostOnLAN . Has this been tested properly? Also are there changes needed on the windows side to match? I only see changes for NetworkLinux and general
This is not my area of expertise but I vote no for isengard.(yes for J* though)

Member

Paxxi commented Apr 30, 2015

I've only gone over this quickly but I'm a bit concerned about how it affects CURL::Parse(we try to find port number by searching for : from the start of the string) and URIUtils::IsHostOnLAN . Has this been tested properly? Also are there changes needed on the windows side to match? I only see changes for NetworkLinux and general
This is not my area of expertise but I vote no for isengard.(yes for J* though)

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Apr 30, 2015

Member

@Paxxi so no expert judgment :)

Member

FernetMenta commented Apr 30, 2015

@Paxxi so no expert judgment :)

@Paxxi

This comment has been minimized.

Show comment
Hide comment
@Paxxi

Paxxi Apr 30, 2015

Member

nope, I leave that to the experts :)

Member

Paxxi commented Apr 30, 2015

nope, I leave that to the experts :)

@MartijnKaijser

This comment has been minimized.

Show comment
Hide comment
@MartijnKaijser

MartijnKaijser May 17, 2015

Member

@FernetMenta @Jalle19 is this still acceptable for 15.0?
jenkins build this please

Member

MartijnKaijser commented May 17, 2015

@FernetMenta @Jalle19 is this still acceptable for 15.0?
jenkins build this please

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 May 18, 2015

Member

@Paxxi's comments haven't been answered yet so I vote no.

Member

Jalle19 commented May 18, 2015

@Paxxi's comments haven't been answered yet so I vote no.

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 18, 2015

Contributor

@Jalle19 now I'm not really sure if mine involvement was expected/asked or not. your last comment triggers such feelings from my side but personally I was still waiting for the 'experts judgement' by a 3rd party - not myself.

as the original comment was saying - that's bare minimum for start for you (multi-platform code) - but is enough for deploying linux distros - thus I would not be rewriting the rest because I feel bored.

I'll be happy to continue and work out other aspects too, just naturally won't be doing that on my own without prior discussion with maintainer's and setting/agreeing on some basics (before other and other code will be rewritten)

Contributor

mk01 commented May 18, 2015

@Jalle19 now I'm not really sure if mine involvement was expected/asked or not. your last comment triggers such feelings from my side but personally I was still waiting for the 'experts judgement' by a 3rd party - not myself.

as the original comment was saying - that's bare minimum for start for you (multi-platform code) - but is enough for deploying linux distros - thus I would not be rewriting the rest because I feel bored.

I'll be happy to continue and work out other aspects too, just naturally won't be doing that on my own without prior discussion with maintainer's and setting/agreeing on some basics (before other and other code will be rewritten)

@Memphiz

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz May 18, 2015

Member

I think it might be a bit too late for isengard at this point.

  1. It looks like it alters behavior of name resolution
  2. It doesn't compile on the other platforms yet:
    android arm&x86:
    NetworkLinux.cpp:31:23: fatal error: ifaddrs.h: No such file or directory
    #include <ifaddrs.h>

iOS & OSX:
/Users/Shared/jenkins/workspace/IOS-ATV2/xbmc/network/linux/NetworkLinux.cpp:128:45: error: use of undeclared identifier 'IFF_LOWER_UP'
m_ifa_flags & (IFF_UP | IFF_LOWER_UP)));

Windows:
DNSNameCache.cpp(64): error C3861: 'inet_aton': identifier not found

After it compiled it might need some more user testing (i mean - we should merge it after I* release and see how feedback is over some months).

Member

Memphiz commented May 18, 2015

I think it might be a bit too late for isengard at this point.

  1. It looks like it alters behavior of name resolution
  2. It doesn't compile on the other platforms yet:
    android arm&x86:
    NetworkLinux.cpp:31:23: fatal error: ifaddrs.h: No such file or directory
    #include <ifaddrs.h>

iOS & OSX:
/Users/Shared/jenkins/workspace/IOS-ATV2/xbmc/network/linux/NetworkLinux.cpp:128:45: error: use of undeclared identifier 'IFF_LOWER_UP'
m_ifa_flags & (IFF_UP | IFF_LOWER_UP)));

Windows:
DNSNameCache.cpp(64): error C3861: 'inet_aton': identifier not found

After it compiled it might need some more user testing (i mean - we should merge it after I* release and see how feedback is over some months).

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 18, 2015

Contributor

@Memphiz

thanks for taking the lead.

I can look (and will) on to osx and (at least code edit without ability to test) the x86/android. hat is
(what is the x86 you build on ? as ifaddrs.h is default part of libc6-dev and deployed in /usr/include. this is valid for wheezy(any arch), jessie(any arch), ubuntu since 12.04...)

(are there some specialties to install for XCode to get master compiling? according to readme.osx it is "just working")

in regards to Windows - unfortunately my last know how is from dos3 and gwbasic.
I just did some MS docs reading before I used those new function calls and type defs. According to it they are supported almost 1:1 in something called "ansi" mode. I looked on examples attached too - some differences in types looked like it is all interfaced somehow to the vc++ containers / native calls. so I assumed win32 guy should be fine.

I will ping again when I push some updates.
btw - if you are fine with the new approach in NetworkLinux.cpp I would re-check & re-adapt whole class (where needed). Speaking of Curl interfacing - curl internals are fine parsing ipv6 ips.
the ipv6 "style" putting ip between [] was chosen to avoid conflicts with ipv4 :ports definitions (and make vintage code work without changes) but I wasn't sure about that when quick looking at CURL::Parse (already pointed before). I will follow up on this.

Honestly - I don't know EXACT workflows in the Network code (yet) - specifically if CURL::Parse is something used for ANY fs/net/xx "destination" parsing or not (meaning what to test in details) - but speaking about it because I already rolled out this commit into stable XBian builds those 20 days ago. that means ANY conflicts/problems will for sure be reported sooner or later.
Also it was merged into Sam's OSMC R2 or R3. ((the message told here is not that code is already bullet-prove, but that it is undergoing wider test already than me and you Memphis)).

Contributor

mk01 commented May 18, 2015

@Memphiz

thanks for taking the lead.

I can look (and will) on to osx and (at least code edit without ability to test) the x86/android. hat is
(what is the x86 you build on ? as ifaddrs.h is default part of libc6-dev and deployed in /usr/include. this is valid for wheezy(any arch), jessie(any arch), ubuntu since 12.04...)

(are there some specialties to install for XCode to get master compiling? according to readme.osx it is "just working")

in regards to Windows - unfortunately my last know how is from dos3 and gwbasic.
I just did some MS docs reading before I used those new function calls and type defs. According to it they are supported almost 1:1 in something called "ansi" mode. I looked on examples attached too - some differences in types looked like it is all interfaced somehow to the vc++ containers / native calls. so I assumed win32 guy should be fine.

I will ping again when I push some updates.
btw - if you are fine with the new approach in NetworkLinux.cpp I would re-check & re-adapt whole class (where needed). Speaking of Curl interfacing - curl internals are fine parsing ipv6 ips.
the ipv6 "style" putting ip between [] was chosen to avoid conflicts with ipv4 :ports definitions (and make vintage code work without changes) but I wasn't sure about that when quick looking at CURL::Parse (already pointed before). I will follow up on this.

Honestly - I don't know EXACT workflows in the Network code (yet) - specifically if CURL::Parse is something used for ANY fs/net/xx "destination" parsing or not (meaning what to test in details) - but speaking about it because I already rolled out this commit into stable XBian builds those 20 days ago. that means ANY conflicts/problems will for sure be reported sooner or later.
Also it was merged into Sam's OSMC R2 or R3. ((the message told here is not that code is already bullet-prove, but that it is undergoing wider test already than me and you Memphis)).

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 18, 2015

Contributor

ach forgot the important - quoting you

1. It looks like it alters behavior of name resolution

I was targeting no change to outside. Can you go into details what I missed ?

Contributor

mk01 commented May 18, 2015

ach forgot the important - quoting you

1. It looks like it alters behavior of name resolution

I was targeting no change to outside. Can you go into details what I missed ?

@Paxxi

This comment has been minimized.

Show comment
Hide comment
@Paxxi

Paxxi May 19, 2015

Member

The choice of going with [] for ipv6 addresses is correct I think as it's commonly used. our CURL class should be fixed in a separate pr I think to add ipv6 parsing and unit testing for it before this PR goes in.

@mk01 we have CURL and Curl, CURL is our path/url class that handles local and remote paths of any kind. Curl is well it's curl as we all know and love :) just to clarify which one we're talking about it's our internal CURL that I'm worried about and it needs ipv6 parsing anyhow.

I can help out with whatever win32 changes are needed once this is in shape to be tested.

To further clarify the win32api has two methods of anything that takes a string one ending with A(ansi) and one with W(unicode/widestring), When docs mention ANSI they mean the method takes string in ascii/oem codepage and the network methods should function mostly as on other platforms but they likely live in another header.

Member

Paxxi commented May 19, 2015

The choice of going with [] for ipv6 addresses is correct I think as it's commonly used. our CURL class should be fixed in a separate pr I think to add ipv6 parsing and unit testing for it before this PR goes in.

@mk01 we have CURL and Curl, CURL is our path/url class that handles local and remote paths of any kind. Curl is well it's curl as we all know and love :) just to clarify which one we're talking about it's our internal CURL that I'm worried about and it needs ipv6 parsing anyhow.

I can help out with whatever win32 changes are needed once this is in shape to be tested.

To further clarify the win32api has two methods of anything that takes a string one ending with A(ansi) and one with W(unicode/widestring), When docs mention ANSI they mean the method takes string in ascii/oem codepage and the network methods should function mostly as on other platforms but they likely live in another header.

@Memphiz

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz May 19, 2015

Member
  1. you only need latest xcode and you need to try to follow the readme as close as possible - that should get it build for osx (and ios)
  2. The diff looks like the shortcut with the comment "// first see if this is already an ip address" is missing now from lookup
  3. android arm&x86 both refers to android. We are compiling using the android native development kit (ndk) - it might have different / subset of headers available only
  4. I already had an adaption of CURL for parsing ipv6 somewhere - trying to dig it up fyi
Member

Memphiz commented May 19, 2015

  1. you only need latest xcode and you need to try to follow the readme as close as possible - that should get it build for osx (and ios)
  2. The diff looks like the shortcut with the comment "// first see if this is already an ip address" is missing now from lookup
  3. android arm&x86 both refers to android. We are compiling using the android native development kit (ndk) - it might have different / subset of headers available only
  4. I already had an adaption of CURL for parsing ipv6 somewhere - trying to dig it up fyi
@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 19, 2015

Contributor

@Paxxi

thx for the quick tour. with the w32 then you will be fine, all by me introduced functions I found as directly supported - normally I would try to adapt the includes for w32 too, but (really) it was so different and with no installation to check against I simply left it as is...

Contributor

mk01 commented May 19, 2015

@Paxxi

thx for the quick tour. with the w32 then you will be fine, all by me introduced functions I found as directly supported - normally I would try to adapt the includes for w32 too, but (really) it was so different and with no installation to check against I simply left it as is...

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 19, 2015

Contributor

@Memphiz

indeed that code I missed initially and force pushed later, so maybe you have link for the obsolete first push?
mk01@741df4a?diff=unified

can you check this please - if we are looking at the same diffo

((what is the requirement of API version to compile on for osx?))

Contributor

mk01 commented May 19, 2015

@Memphiz

indeed that code I missed initially and force pushed later, so maybe you have link for the obsolete first push?
mk01@741df4a?diff=unified

can you check this please - if we are looking at the same diffo

((what is the requirement of API version to compile on for osx?))

@Memphiz

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz May 19, 2015

Member

Yes we are looking at the same diff.

Former approach:

  • try to convert hostname to ip address - on success craft a string from the ip address
    New approach
  • try to convert hostname to ip or ip6 address - on success return the hostname itself as ip address

The new approach is fine for ipv4 addresses i think (as there is only one allowed ip format) - but the ipv6 approach might give some issues (might be out of scope of this PR though as its a new use case for dns lookup here). The former code ensured that the returned ip was properly formatted, the new approach returns the hostname if it contains an ipv6 address. Based on the man page of inet_pton there are multiple supported ipv6 formats. So whatever the format was in hostname will be the format in the returned ip address. Not sure if we would need to return a generic format here. Or even add the "[" "]" to it already - so the caller can use the returned address inside of URLs.

Example:

  • CDNSNameCache::Lookup("192.168.1.100", strIp);
    string myNewUrl = "nfs://" + strIp + "/myexport";

For ipv4 this is fine.

  • CDNSNameCache::Lookup("2607:f8b0:4010:801:0:0:0:1018", strIp6);
    string myNewUrl = "nfs://" + strIp6 + "/myexport";

For ipv6 this would be wrong i think (missing brackets)...

Member

Memphiz commented May 19, 2015

Yes we are looking at the same diff.

Former approach:

  • try to convert hostname to ip address - on success craft a string from the ip address
    New approach
  • try to convert hostname to ip or ip6 address - on success return the hostname itself as ip address

The new approach is fine for ipv4 addresses i think (as there is only one allowed ip format) - but the ipv6 approach might give some issues (might be out of scope of this PR though as its a new use case for dns lookup here). The former code ensured that the returned ip was properly formatted, the new approach returns the hostname if it contains an ipv6 address. Based on the man page of inet_pton there are multiple supported ipv6 formats. So whatever the format was in hostname will be the format in the returned ip address. Not sure if we would need to return a generic format here. Or even add the "[" "]" to it already - so the caller can use the returned address inside of URLs.

Example:

  • CDNSNameCache::Lookup("192.168.1.100", strIp);
    string myNewUrl = "nfs://" + strIp + "/myexport";

For ipv4 this is fine.

  • CDNSNameCache::Lookup("2607:f8b0:4010:801:0:0:0:1018", strIp6);
    string myNewUrl = "nfs://" + strIp6 + "/myexport";

For ipv6 this would be wrong i think (missing brackets)...

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 19, 2015

Contributor

@Memphiz

the += is probably some kind of (evidently) wrong habit when assigning value to std::string. thanks for pointing.

what particular part makes you say that hostname is returned (instead of IP) ?

apropo IPs. I was just going through NetworkLinux.cpp to check/align all it's content and realised there are functions defined with u32 as IP parameter.
maybe we could agree on how to pass "host" parameter for the future.
I can even imagine a super tiny class as abstract wrapper around sockaddr_in/in6 respectively.

Contributor

mk01 commented May 19, 2015

@Memphiz

the += is probably some kind of (evidently) wrong habit when assigning value to std::string. thanks for pointing.

what particular part makes you say that hostname is returned (instead of IP) ?

apropo IPs. I was just going through NetworkLinux.cpp to check/align all it's content and realised there are functions defined with u32 as IP parameter.
maybe we could agree on how to pass "host" parameter for the future.
I can even imagine a super tiny class as abstract wrapper around sockaddr_in/in6 respectively.

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 19, 2015

Contributor

@Memphiz @Paxxi

just pushed few changes.

  • code should now compile with all platforms (but w32)
  • unfortunately it looks that Android NDK doesn't provide implementation of ifaddrs yet

via google I came over it's implementation https://github.com/kmackay/android-ifaddrs which looks to be used on Android for this purpose. it is integrated in the commit - although I'm not sure about XBMC's policy with such injections - it is there for now to be able moving forward.

@Memphiz
I tested compilation for osx, but not roid/x86. can you ask Jenkins to do so ?

Contributor

mk01 commented May 19, 2015

@Memphiz @Paxxi

just pushed few changes.

  • code should now compile with all platforms (but w32)
  • unfortunately it looks that Android NDK doesn't provide implementation of ifaddrs yet

via google I came over it's implementation https://github.com/kmackay/android-ifaddrs which looks to be used on Android for this purpose. it is integrated in the commit - although I'm not sure about XBMC's policy with such injections - it is there for now to be able moving forward.

@Memphiz
I tested compilation for osx, but not roid/x86. can you ask Jenkins to do so ?

@Memphiz

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz May 20, 2015

Member

Look at my example again and then i mean this assignment

mk01@a18e65c#diff-3c53759a9e37920d7538e7aa60223fcaR74

In this case hostname was already an ipv6 and gets assigned 1:1 to the ip address. But there is no common format guaranteed for the ipv6 in that case. (no issue for ipv4 - this is something new because v6 can have multiple formats)

up to @koying if its ok to backport lonux code to droid

jenkins build this please

Member

Memphiz commented May 20, 2015

Look at my example again and then i mean this assignment

mk01@a18e65c#diff-3c53759a9e37920d7538e7aa60223fcaR74

In this case hostname was already an ipv6 and gets assigned 1:1 to the ip address. But there is no common format guaranteed for the ipv6 in that case. (no issue for ipv4 - this is something new because v6 can have multiple formats)

up to @koying if its ok to backport lonux code to droid

jenkins build this please

@koying

This comment has been minimized.

Show comment
Hide comment
@koying

koying May 20, 2015

Contributor

I have no idea on the status of IPV6 in android/NDK and have my hands full, so I'd suggest to ifdef out android for now, if possible, to not be blocking.

Contributor

koying commented May 20, 2015

I have no idea on the status of IPV6 in android/NDK and have my hands full, so I'd suggest to ifdef out android for now, if possible, to not be blocking.

@Memphiz

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz May 20, 2015

Member

@mk01 you stated you compiled this successfully on osx? Well jenkins says no.

Just search for "NetworkLinux.cpp" in the logs:

http://jenkins.kodi.tv/job/OSX-64/4503/console
http://jenkins.kodi.tv/job/OSX-32/4320/console
http://jenkins.kodi.tv/job/IOS-ATV2/4052/console

Member

Memphiz commented May 20, 2015

@mk01 you stated you compiled this successfully on osx? Well jenkins says no.

Just search for "NetworkLinux.cpp" in the logs:

http://jenkins.kodi.tv/job/OSX-64/4503/console
http://jenkins.kodi.tv/job/OSX-32/4320/console
http://jenkins.kodi.tv/job/IOS-ATV2/4052/console

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 May 20, 2015

Contributor

@Memphiz

compiled - and forgot to include it's changes into commit (because having different source dirs for different platforms). doing it right away.

Contributor

mk01 commented May 20, 2015

@Memphiz

compiled - and forgot to include it's changes into commit (because having different source dirs for different platforms). doing it right away.

@Paxxi

This comment has been minimized.

Show comment
Hide comment
@Paxxi

Paxxi May 20, 2015

Member

@Memphiz if you mean the truncation of zeroes or the mixed : and . notation I don't think that's something we need to care about, all three are standardized so any ipv6-stack we run on should handle all three fine.

Thinking about which format would be best really as they all have pros and cons.

  1. returning plain ip without []. Pro: we can hand it directly to socket methods and it should work. Con: easy to screw up formatting urls.
  2. return ip enclosed in []. Reverse of 1
  3. encapsulate in a class as @mk01 mentioned, possibly something like
class CIP
{
public:
  std::string GetPlainIP();
  std::string GetFormatted();
private:
std::string m_ip;
int m_type;
}

not sure if we would need to expose the type, would need to look into where it's used before that decision is made.

Member

Paxxi commented May 20, 2015

@Memphiz if you mean the truncation of zeroes or the mixed : and . notation I don't think that's something we need to care about, all three are standardized so any ipv6-stack we run on should handle all three fine.

Thinking about which format would be best really as they all have pros and cons.

  1. returning plain ip without []. Pro: we can hand it directly to socket methods and it should work. Con: easy to screw up formatting urls.
  2. return ip enclosed in []. Reverse of 1
  3. encapsulate in a class as @mk01 mentioned, possibly something like
class CIP
{
public:
  std::string GetPlainIP();
  std::string GetFormatted();
private:
std::string m_ip;
int m_type;
}

not sure if we would need to expose the type, would need to look into where it's used before that decision is made.

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 Jun 7, 2015

Contributor

@Memphiz @Paxxi

colleagues.
if you find time, feel free to look into latest push. nothing is redesigned, just continued towards (full) ipv6 support. for now no address wrapping , I used a string (plain, no []) as It looks it is not needed (at the level of Network class.

  • I rewrote the provided functions to use native format (sockaddr_in*) and string (or u32 from past ipv4 implementation) are currently used only to pass a parameter into function or result out of it. what was possible (not platform dep) I moved out to CNetwork scope (to be reused by other platforms(win)).
  • I also tried to update most of the LinuxNetwork dependent functions to work with ipv6 transparently (so no change from callers needed (beside the functions which do SET system parameters). I will continue updating the "passive" (if still any to be updated), but I rather not touch the active ones.
  • back to 1) - all the helper functions are universal now (doesn't matter if ipv6/v4 is passed to them - again string is used as type (for now). previous ipv4 only (u32 param) are kept but are just a wrappers to the func(string xxx) new ones.

There is also a lot of "net/ip/stuff" handling code outside of ./xbmc/network which I'm trying to get rid of - slowly - when found.

current code builds on osx and arm(linux). if there was osx specifics i tried to match at compatibility level with BSD. all the new code is making use of only the three functions (inet_ntop/pton and getifaddrs) which are supported * with only the one exception of getifaddrs on Android SDK). it's implementation (xbmc/network/linux/android-ifaddrs) is wrapper around netlink calls (and that is how getifaddrs is provided on others too (linux, osx only natively already).

@Paxxi just one remark here, I have not read win32 specs in detail about getifaddrs, but there is a difference between BSD(osx) and Linux implementation - BSD returns one structure/interface with iterating over its IPs in ->next, Linux returns structure/address. Practically it has just one consequence, BSD results can contain NULLs. I tried to make all supports functions (in CNetwork) be testing that, so if Win is the case with NULLs, you don't have to handle specially - it should work fine. But if you will get any SEGVs, try to check the class provided functions first. Might be I missed something.

@Memphiz
to wrap the getifaddrs<->android problem - It is no time to change Android back to prev code (if you decide not to use the provided external implementation). There are just TWO calls to it. Specifically DNSCache (resolving IPv6 addresses) and in NetworkLinux - queryinterfaces list(); On the other hand, because there are just TWO calls to it can be filly tested, in case of issue easy identified so the risk is very low. And it's use quite much harmonised the platform code (no more text / ext util output parsing and #ifdefs).

I have not yet, but will do get my hands on Android docs - how do the platform intents to access networking info in (suitable) way (of course dual-ok). I don't want to believe that only by using the lowlevel NetLink interface which is needing very much code to write for too little effect (for purpose we need it).
Ok, so google shows google tells, apparently there is no real pressure for support because "embedded world doesn't need ipv6". without to comment that particular point, this brings a little trouble into whole x86(linux) being compiled as Android. As pure info from dev boards I quickly looked over - what tends to work better is pure Ipv6 setups, what tends not to work (in smaller or greater extends) are dual stack configs of any type - with tunnel setups & autoconfiguration mechanisms at worst. But staying at this commit only - this has no impact on code existing at now (or it's functionality).

@Paxxi no pushing or something, but the "core" changes are done. That means (any) work you can do on the Win side, will not get lost / changed anymore.
Theoretically what should be only needed is to try to compile (specially DNSCache & Network.cpp at ./xbmc/network which use the new triplet of functions - actually there should be no specific Vx handling anywhere else. If does, please let me know).
If you get it compiled, until IPv6 for Win is really needed, no further changes should be needed - CNetwork class now expects by default SupportV6() be false and all the Dual-stack functionality should recognise that and work with ipv4 as it was before.

br,
mk

Contributor

mk01 commented Jun 7, 2015

@Memphiz @Paxxi

colleagues.
if you find time, feel free to look into latest push. nothing is redesigned, just continued towards (full) ipv6 support. for now no address wrapping , I used a string (plain, no []) as It looks it is not needed (at the level of Network class.

  • I rewrote the provided functions to use native format (sockaddr_in*) and string (or u32 from past ipv4 implementation) are currently used only to pass a parameter into function or result out of it. what was possible (not platform dep) I moved out to CNetwork scope (to be reused by other platforms(win)).
  • I also tried to update most of the LinuxNetwork dependent functions to work with ipv6 transparently (so no change from callers needed (beside the functions which do SET system parameters). I will continue updating the "passive" (if still any to be updated), but I rather not touch the active ones.
  • back to 1) - all the helper functions are universal now (doesn't matter if ipv6/v4 is passed to them - again string is used as type (for now). previous ipv4 only (u32 param) are kept but are just a wrappers to the func(string xxx) new ones.

There is also a lot of "net/ip/stuff" handling code outside of ./xbmc/network which I'm trying to get rid of - slowly - when found.

current code builds on osx and arm(linux). if there was osx specifics i tried to match at compatibility level with BSD. all the new code is making use of only the three functions (inet_ntop/pton and getifaddrs) which are supported * with only the one exception of getifaddrs on Android SDK). it's implementation (xbmc/network/linux/android-ifaddrs) is wrapper around netlink calls (and that is how getifaddrs is provided on others too (linux, osx only natively already).

@Paxxi just one remark here, I have not read win32 specs in detail about getifaddrs, but there is a difference between BSD(osx) and Linux implementation - BSD returns one structure/interface with iterating over its IPs in ->next, Linux returns structure/address. Practically it has just one consequence, BSD results can contain NULLs. I tried to make all supports functions (in CNetwork) be testing that, so if Win is the case with NULLs, you don't have to handle specially - it should work fine. But if you will get any SEGVs, try to check the class provided functions first. Might be I missed something.

@Memphiz
to wrap the getifaddrs<->android problem - It is no time to change Android back to prev code (if you decide not to use the provided external implementation). There are just TWO calls to it. Specifically DNSCache (resolving IPv6 addresses) and in NetworkLinux - queryinterfaces list(); On the other hand, because there are just TWO calls to it can be filly tested, in case of issue easy identified so the risk is very low. And it's use quite much harmonised the platform code (no more text / ext util output parsing and #ifdefs).

I have not yet, but will do get my hands on Android docs - how do the platform intents to access networking info in (suitable) way (of course dual-ok). I don't want to believe that only by using the lowlevel NetLink interface which is needing very much code to write for too little effect (for purpose we need it).
Ok, so google shows google tells, apparently there is no real pressure for support because "embedded world doesn't need ipv6". without to comment that particular point, this brings a little trouble into whole x86(linux) being compiled as Android. As pure info from dev boards I quickly looked over - what tends to work better is pure Ipv6 setups, what tends not to work (in smaller or greater extends) are dual stack configs of any type - with tunnel setups & autoconfiguration mechanisms at worst. But staying at this commit only - this has no impact on code existing at now (or it's functionality).

@Paxxi no pushing or something, but the "core" changes are done. That means (any) work you can do on the Win side, will not get lost / changed anymore.
Theoretically what should be only needed is to try to compile (specially DNSCache & Network.cpp at ./xbmc/network which use the new triplet of functions - actually there should be no specific Vx handling anywhere else. If does, please let me know).
If you get it compiled, until IPv6 for Win is really needed, no further changes should be needed - CNetwork class now expects by default SupportV6() be false and all the Dual-stack functionality should recognise that and work with ipv4 as it was before.

br,
mk

@mk01

This comment has been minimized.

Show comment
Hide comment
@mk01

mk01 Jun 7, 2015

Contributor

very small howto

  • until you internally decide how to manage "addresses" for future, CNetwork(or nativeInterface) provides some functions (which would be a in-class methods otherwise).
  • *iface has isIPv4() and isIPv6() public methods - to tell if current iteration(if) is configured with ipv6 or ipv4 address.
  • ConvIPv6(string,*sockaddr) and ConvIPv4(string, *sockaddr) are dual purpose - they check validity of tested "string" for the particular stack, if *sockaddr is supplied, string is directly also converted into that struct (directly usable at network stack level functions).
  • SupportsIPv6() - implementation specific - if set to false, IPv6 code in universal functions is skipped (and any calls to v6XXX methods will result in NULL or FALSE).
  • CanoniseIPv6(string) - "shortens" ipv6 address according the RFC. if non-valid string, the same without change is returned
  • PrefixLengthIPv6(string) - returns LocalPrefix (int from 0 to 128) for full size ipv6 netmask
  • GetStrIp() - returns IP to ANY hostname, canonised in case of ipv6 / full size (x.x.x.x) for ipv4
Contributor

mk01 commented Jun 7, 2015

very small howto

  • until you internally decide how to manage "addresses" for future, CNetwork(or nativeInterface) provides some functions (which would be a in-class methods otherwise).
  • *iface has isIPv4() and isIPv6() public methods - to tell if current iteration(if) is configured with ipv6 or ipv4 address.
  • ConvIPv6(string,*sockaddr) and ConvIPv4(string, *sockaddr) are dual purpose - they check validity of tested "string" for the particular stack, if *sockaddr is supplied, string is directly also converted into that struct (directly usable at network stack level functions).
  • SupportsIPv6() - implementation specific - if set to false, IPv6 code in universal functions is skipped (and any calls to v6XXX methods will result in NULL or FALSE).
  • CanoniseIPv6(string) - "shortens" ipv6 address according the RFC. if non-valid string, the same without change is returned
  • PrefixLengthIPv6(string) - returns LocalPrefix (int from 0 to 128) for full size ipv6 netmask
  • GetStrIp() - returns IP to ANY hostname, canonised in case of ipv6 / full size (x.x.x.x) for ipv4
@Memphiz

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz Jun 7, 2015

Member

Sounds like you did some great stuff (only read your comment yet). Some busy days coming up here - i try to take a look as soon as time permits :).

Member

Memphiz commented Jun 7, 2015

Sounds like you did some great stuff (only read your comment yet). Some busy days coming up here - i try to take a look as soon as time permits :).

mkreisl and others added some commits Nov 2, 2015

[NetworkLinux] - align the behaviour with W32 implementation and load
only interfaces with flag IFF_UP (interface has (some) configuration
and is UP - bind() will not fail)

(this means non empty interfaces list is sufficient prerequisite
to start network services without errors)
[NetWatch] - move the core code into CNetwork, keep only NetLink
specific handler in LinuxNetwork code.

Prepare for easy w32 watcher integration.
[NetworkLinux] - OSX/FreeBSD fixes
- fix FreeBSD/OSX MacAddr fetch
- fix NetworkInterfaceLinux::IsConnected()
[CNetwork] - stop and start NetworkUpdater thread with OnSleep/OnWake
    announcements. This avoids unnecessary reconfigurations when IFs
are changing back to IFF_UP as part of resuming process.
[CNetwork, WakeOnLan, CDNSNameCache]
- remove network api calls from wakeonlan, use new CNetwork API
- move HostToIp() to CDNSNameCache / rename to Lookup(), adapt
  calls accordingly
- remove Lookup() failure logging from WakeOnLane (as this
  is logged in CDNSNameCache itself)
[CDNSNameCache] Update to master
- remove announcer use from CDNSNameCache
- flush name cache from Network on services start/restart

(cherry picked from commit 36666d3173633f125f7d3494a8e3c90f365a9d54)
@Razzeee

This comment has been minimized.

Show comment
Hide comment
@Razzeee

Razzeee Apr 13, 2017

Member

We might want to move ahead with this now, as we're in pre-alpha status right now.

Member

Razzeee commented Apr 13, 2017

We might want to move ahead with this now, as we're in pre-alpha status right now.

@Razzeee

This comment has been minimized.

Show comment
Hide comment
@Razzeee

Razzeee Sep 16, 2017

Member

I would still like to see this merged, as it's early in the v18 cycle and even if we break nightlies for some days, so be it.

Member

Razzeee commented Sep 16, 2017

I would still like to see this merged, as it's early in the v18 cycle and even if we break nightlies for some days, so be it.

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Sep 16, 2017

Member

this needs more than just a single additional round. first it needs a rebase, ...

Member

FernetMenta commented Sep 16, 2017

this needs more than just a single additional round. first it needs a rebase, ...

@jenkins4kodi

This comment has been minimized.

Show comment
Hide comment
@jenkins4kodi

jenkins4kodi Nov 19, 2017

Contributor

@mk01 this needs a rebase

Contributor

jenkins4kodi commented Nov 19, 2017

@mk01 this needs a rebase

@MartijnKaijser

This comment has been minimized.

Show comment
Hide comment
@MartijnKaijser

MartijnKaijser Jun 24, 2018

Member

I will close this PR. If there's interest please open a new one (with required changes if needed).

Member

MartijnKaijser commented Jun 24, 2018

I will close this PR. If there's interest please open a new one (with required changes if needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment