Skip to content

Fix build for macOS where endian.h is not available #1983

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cho-m
Copy link

@cho-m cho-m commented Nov 13, 2024

It is possible other platforms were impacted as the header/functions aren't that portable.

Specific errors:

common/network/NetworkUtils.cpp:166:2: error: "No be64toh for NetworkToHost, please report this."
  166 | #error "No be64toh for NetworkToHost, please report this."
      |  ^
common/network/NetworkUtils.cpp:182:2: error: "No be64toh for NetworkToHost, please report this."
  182 | #error "No be64toh for NetworkToHost, please report this."
      |  ^
common/network/NetworkUtils.cpp:206:2: error: "No htobe64 for HostToNetwork, please report this."
  206 | #error "No htobe64 for HostToNetwork, please report this."
      |  ^
common/network/NetworkUtils.cpp:214:2: error: "No htobe64 for HostToNetwork, please report this."
  214 | #error "No htobe64 for HostToNetwork, please report this."
      |  ^
4 errors generated.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Thanks for this @cho-m and sorry for breaking things for you.

Rather than redefining be64toh and htobe64, we prefer to do ifdefs to handle the different cases. We should probably also add a header check for libkern/OSByteOrder.h and use that rather than APPLE to switch the behaviour.

Comment on lines +65 to +66
#define be64toh(x) OSSwapBigToHostInt64(x)
#define htobe64(x) OSSwapHostToBigInt64(x)
Copy link
Member

Choose a reason for hiding this comment

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

So remove this:

Suggested change
#define be64toh(x) OSSwapBigToHostInt64(x)
#define htobe64(x) OSSwapHostToBigInt64(x)

Comment on lines +63 to +64
#elif defined(__APPLE__)
#include <libkern/OSByteOrder.h>
Copy link
Member

Choose a reason for hiding this comment

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

Switch this to including OSByteOrder.h if its present.

Comment on lines -163 to +171
#ifdef HAVE_ENDIAN_H
#if defined(HAVE_ENDIAN_H) || defined(__APPLE__)
return be64toh(value);
#else
#error "No be64toh for NetworkToHost, please report this."
#endif // HAVE_ENDIAN_H
#endif // defined(HAVE_ENDIAN_H) || defined(__APPLE__)
Copy link
Member

Choose a reason for hiding this comment

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

Then switch this, and the following cases, to something more like

#ifdef USE_SYSCTL_FOR_DEFAULT_ROUTE
return GetDefaultRouteWithSysctl(if_index, default_gateway);
#elif defined(USE_NETLINK_FOR_DEFAULT_ROUTE)
return GetDefaultRouteWithNetlink(if_index, default_gateway);
#elif defined(_WIN32)
ULONG size = 4096;
PMIB_IPFORWARDTABLE forward_table =
reinterpret_cast<PMIB_IPFORWARDTABLE>(malloc(size));
DWORD result = GetIpForwardTable(forward_table, &size, TRUE);
if (result == NO_ERROR) {
for (unsigned int i = 0; i < forward_table->dwNumEntries; ++i) {
if (forward_table->table[i].dwForwardDest == 0) {
*default_gateway =
IPV4Address(forward_table->table[i].dwForwardNextHop);
*if_index = forward_table->table[i].dwForwardIfIndex;
}
}
free(forward_table);
return true;
} else {
OLA_WARN << "GetIpForwardTable failed with " << GetLastError();
return false;
}
#else
#error "DefaultRoute not implemented for this platform, please report this."
// TODO(Peter): Do something else on machines without Netlink
// No Netlink, can't do anything
return false;
#endif // USE_SYSCTL_FOR_DEFAULT_ROUTE
(but I don't think we need the extra wrapper functions here).

@peternewman peternewman added this to the 0.11.0 milestone Mar 23, 2025
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.

2 participants