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

[iOS] - Implement CNetworkIOS implementation for darwin_embedded platforms #16607

Merged
merged 2 commits into from Oct 10, 2019

Conversation

fuzzard
Copy link
Contributor

@fuzzard fuzzard commented Sep 11, 2019

Description

This fixes/extends Memphiz's previous PR #14344
Have relocated files to ios-common, as the intent will be for it to be used for tvos as well.
This also enables ipv6 functionality for those network functions
Note: iOS does NOT allow the finding of mac address for interfaces. This was deprecated in ios7, with a hard OS change in ios 11. The method that did work in ios7-10 was to look at the arp table, but apple now hijack this, and return 02:00:00:00:00:00. Decision was made to just return empty string.
This also means that GetHostMacAddress() is not able to be implemented for WOL. With that change, there is no longer a need for the ioshacks.h header (was only used for mac address functionality)
The implementation also fixes a reported issue from the forums - https://forum.kodi.tv/showthread.php?tid=341593&pid=2888545#pid2888545

Motivation and Context

Saw a forum post with a user being a bit of a jerk. In spite of that, thought id get memphiz's PR up to the current state, and have cleaned up a bit more.

How Has This Been Tested?

ipad -> sys info responds with updating ip/dns/gateway correctly on network change
iphone -> tested on wifi/cell with both ipv4/6, and info updates correctly

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@fuzzard fuzzard force-pushed the ios_network branch 6 times, most recently from 960eb0b to 26d66b9 Compare September 15, 2019 07:37
@fuzzard
Copy link
Contributor Author

fuzzard commented Sep 15, 2019

I believe this is in its final state now. Have set a priority ordering for return of GetFirstinterfaceConnected to be VPN->Wifi->Cell. System information now updates ip/dns/gateway with consistent info on network changes, and works across both ipv4/ipv6 networks.

Hoping i can now get a full jenkins build

Jenkins build this please

@kambala-decapitator
Copy link
Contributor

Jenkins build this please

xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
xbmc/platform/darwin/ios-common/network/NetworkIOS.mm Outdated Show resolved Hide resolved
@fuzzard fuzzard force-pushed the ios_network branch 2 times, most recently from 4a25058 to 8980ba6 Compare September 25, 2019 11:04
@fuzzard fuzzard force-pushed the ios_network branch 2 times, most recently from 36a5e88 to 455bf0e Compare October 2, 2019 06:05
@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 2, 2019

Ok, cleaned up a few more, fixed up a malloc crash that was occurring with the first iteration as well.

Jenkins build this please

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 2, 2019

Todo's added as requested.
Fixed up one final use case of kodi started in airplane mode, no interfaces are available, so wouldnt populate once turned back on. This will also handle hard adapter changes (lighting->ethernet adapter connected/disconnected)

This rework has also been confirmed to fix the issue noted on the forums
https://forum.kodi.tv/showthread.php?tid=341593&pid=2888545#pid2888545

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 2, 2019
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 2, 2019
@fuzzard fuzzard changed the title [Ios] - network nameserver and gateway [iOS] - Implement CNetworkIOS implementation for darwin_embedded platforms Oct 2, 2019
@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 2, 2019

Have updated PR/commit messages.

Please note, this does fix a reported bug in Leia, however i do not intend to backport. I believe the change is too large, and although i do not believe there are any regressions, i dont particularly want to handle it if something does come up in leia.

Any further changes required to get this in?

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 3, 2019

jenkins build this please

@kambala-decapitator
Copy link
Contributor

Caught a crash while running on iPad Air 2 with iOS 13.1.2. Last lines of stack trace:

CSysInfoJob::DoWork() (in Kodi) (SystemInfo.cpp:256)
CSysInfoJob::GetMACAddress() (in Kodi) (SystemInfo.cpp:284)
CNetworkIOS::GetFirstConnectedInterface() (in Kodi) (NetworkIOS.mm:314)
CNetworkIOS::GetFirstConnectedInterface() (in Kodi) (NetworkIOS.mm:316)

Full log: Kodi 03-10-2019, 10-32.crash.txt

@kambala-decapitator
Copy link
Contributor

I think that defined(HAS_IOS_NETWORK) should be removed from SystemInfo.cpp:283 regardless of the crash

@Rechi
Copy link
Member

Rechi commented Oct 3, 2019

No, why do you think so?

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 3, 2019

In Memphiz's original PR this was based off, it was raised the question about if there were any platforms that were not covered by that ifdef. I would guess its more likely that we remove the ifdef completely

@Rechi
Copy link
Member

Rechi commented Oct 3, 2019

Removing the whole ifdef could be possible, but I read your previous comment that you only want to remove || defined(HAS_IOS_NETWORK).

@kambala-decapitator
Copy link
Contributor

we know beforehand that iOS::GetMAC() returns an empty string, so why do useless work by retrieving interface and then get the very same result? I'd just make a shortcut and leave a todo.

@Rechi
Copy link
Member

Rechi commented Oct 3, 2019

No, SystemInfo doesn't have to know anything about the implementation details of CNetworkInterfaceIOS ::GetMacAddress().

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 3, 2019

Every default platform is covered by that ifdef (android, freebsd, osx, linux = linux_network, win is win, then ios.)

Do you want the ifdef removed in a separate commit?

@Rechi
Copy link
Member

Rechi commented Oct 3, 2019

As the ifdef currently is true for everything, sure.

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 3, 2019

Have added a separate commit removing that ifdef. Have a look and let me know if you want it done a different way (ie remove the else and just have the return ""; as the fall through).

Jenkins keeps giving me empty builds, so hoping this request will work.

jenkins build this please

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 5, 2019
@Rechi
Copy link
Member

Rechi commented Oct 6, 2019

There is nothing in the Kodi coding standard which allows or disallows else statements after return. I personally wouldn't have added the else.

@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Oct 7, 2019
@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 7, 2019

jenkins build this please

@fuzzard
Copy link
Contributor Author

fuzzard commented Oct 10, 2019

Any further comments? I'll look to merge it in the next 24hrs if all ok.

@Rechi
Copy link
Member

Rechi commented Oct 10, 2019

Please rebase and use the new formatting rules of #16701.

Ifdef was always true for all current platforms, making it redundant.
…menation

Removes darwin_embedded network implementation from standard posix network implementation
Handles all CNetworkBase overloads specific to darwin_embedded platforms.
As of ios11, it is not possible to retrieve interface MAC addresses, or to retrieve info
from the devices ARP table. WOL/WakeOnAccess features are not possible without these.
MAC address access was actually deprecated in ios7.
@Memphiz
Copy link
Member

Memphiz commented Oct 11, 2019

Nice work thx :)

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

Successfully merging this pull request may close these issues.

None yet

5 participants