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

[Windows] Fix Discovery of MAC Address #25111

Merged
merged 2 commits into from
May 4, 2024
Merged

Conversation

CrystalP
Copy link
Contributor

@CrystalP CrystalP commented May 3, 2024

Description

Fix the CNetworkInterfaceWin32::GetHostMacAddress() function on Windows. It seems to have stopped working with PR #14369 which added support for IPv6.
A pointer dereference was missing and the wrong data was passed to the Windows API ResolveIpNetEntry2() for host discovery. It always failed with error 87 (ERROR_INVALID_PARAMETER).

For style, I could have gone the way below instead for the same result, but is it a safe way to copy structs of various sizes?

neighborIp.Address.Ipv4 = *reinterpret_cast<const struct sockaddr_in*>(host);
...
neighborIp.Address.Ipv6 = *reinterpret_cast<const struct sockaddr_in6*>(host);

Also added a small optimization to look for the host in the OS cache before hitting the wire with a resolution request.

I think this can be backported.

Motivation and context

Should fix the broken WOL issue reported in forum https://forum.kodi.tv/showthread.php?tid=377218 and #25058.

How has this been tested?

Windows 10, IPv4.
The hosts are resolved when activating the WOL setting of Kodi, toast messages appear and a wakeonlan.xml file with valid addresses is generated.

IPv6 support is not in a shape that allows testing of MAC resolution.

What is the effect on users?

Working WOL function on Windows.

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)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • 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

@CrystalP
Copy link
Contributor Author

CrystalP commented May 3, 2024

Jenkins build this please

Copy link
Member

@thexai thexai left a comment

Choose a reason for hiding this comment

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

Good find!

Tested on windows 11 and is working.

@@ -248,7 +248,7 @@ bool CNetworkWin32::PingHost(const struct sockaddr& host, unsigned int timeout_m

bool CNetworkInterfaceWin32::GetHostMacAddress(unsigned long host, std::string& mac) const
{
struct sockaddr sockHost;
struct sockaddr sockHost = {};
Copy link
Member

@thexai thexai May 3, 2024

Choose a reason for hiding this comment

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

Suggested change
struct sockaddr sockHost = {};
sockaddr sockHost = {};

Seems rare the use of "struct" word here. I mean struct is for define a struct but here is declare a struct already defined in Windows headers.

Same thing in CNetworkWin32::PingHost and CNetworkInterfaceWin32::GetHostMacAddress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I didn't question the unnecessary C syntax. That's the reason why the format bot insisted on weird formatting in the initialization. Changed, but it's not broken and I'm not changing the other ones of the file.

sockaddr structure was filled with pointer value instead of the data pointed to.
@CrystalP CrystalP merged commit 0894898 into xbmc:master May 4, 2024
2 checks passed
@CrystalP CrystalP deleted the fix-macdiscovery branch May 4, 2024 16:54
@Zebraitis
Copy link

Zebraitis commented May 6, 2024

May 5th v22 nightly User Acceptance Test results: The server wakes from sleep. But...

  • It finds the MAC address if the Win11 NAS server shared sources are defined by IP Address.

  • It fails MAC discovery if the Win11 NAS server shared sources are defined by samba name.

For me, this is a huge improvement. However, it does throw up the red MAC Discovery Fail message, which will likely cause confusion for some users.

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.

Wake On Lan - MAC discovery problem with KODI running in Windows 11 with Kodi v21
3 participants