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

NetworkManager should use correct field offset on SunOS and friends #23

Merged
merged 1 commit into from
May 20, 2022
Merged

Conversation

sjorge
Copy link
Contributor

@sjorge sjorge commented Apr 16, 2022

♻️ Current situation

See #21, but basically the field offset in the arp command output on SunOS-like systems does not match 100% with OpenBSD systems, resulting in NetworkManager.getOpenBSD_SUNOS_NetworkInterfaces() not returning any interfaces on SunOS-like systems. Making CIAO kind of useless.

💡 Proposed solution

Overall the code in NetworkManager.getOpenBSD_SUNOS_NetworkInterfaces() looks good, it just needs a different field offset between OpenBSD and SunOS.

When os.platform() returns sunos (which it does on Oracle Solaris and illumos distros (OpenSolaris continuation)), we simple use a different offset. The default is left as 2 which was the old value.

⚙️ Release Notes

CIAO now works properly on SunOS-like systems and will be able to listen on an interface that is not lo0.

➕ Additional Information

n/a

Testing

Made sure I could run npm run test and that the tests still pass

[hyperon :: sjorge][/tmp/ciao][sunos ✔]
[■]$ npm run test

> @homebridge/ciao@1.1.3 test
> jest

 PASS  src/util/sorted-array.spec.ts (5.396 s)
 PASS  src/util/dns-equal.spec.ts (5.327 s)
 PASS  src/coder/DNSLabelCoder.spec.ts (5.845 s)
 PASS  src/util/domain-formatter.spec.ts (5.926 s)
 PASS  src/NetworkManager.spec.ts (6.112 s)
 PASS  src/util/tiebreaking.spec.ts (6.27 s)
 PASS  src/coder/Question.spec.ts (6.225 s)
 PASS  src/MDNSServer.spec.ts (6.32 s)
 PASS  src/coder/DNSPacket.spec.ts (6.473 s)
 PASS  src/coder/ResourceRecord.spec.ts (6.468 s)
 PASS  src/responder/TruncatedQuery.spec.ts (7.016 s)

I also switched my nrchkb node-red setup over to the CIAO backend instead of older bonjour backend. (Wanting to switch is what made me discover CIAO was broken)

I've been running said setup for about a month now with CIAO and the change in this PR.

Reviewer Nudging

Not sure, it's a simple offset change, the original issue I filed lists the output of the execute arp command for an illumos based distro and from Oracle Solaris.

getOpenBSD_SUNOS_NetworkInterfaces() is supose to work on both OpenBSD and SunOS like operating systemens.

However the field offset is different between OpenBSD and SunOS. I verified the arp output is the same on Oracle Solaris 11.x, OmniOS, SmartOS, ...
@adriancable
Copy link
Contributor

@sjorge - I have wondered for a while - why does Ciao use arp in the first place, and only os.networkInterfaces() as a fallback?

I must be missing something (in which case, please share!) because this doesn't make a lot of sense to me. Even if os.networkInterfaces(), due to some Node quirk, doesn't show all valid interfaces, these 'extra' interfaces would not then be usable from Node afterwards anyway. So why don't we just use os.networkInterfaces() as the primary? No need for arp, no need for parsing, no issue with SunOS.

@sjorge
Copy link
Contributor Author

sjorge commented Apr 21, 2022

I have no idea, just went for the most simple fix that I could get to work.

@adriancable
Copy link
Contributor

@sjorge - I think there is a much simpler fix though. Line 365 of NetworkManager.ts (original version) is currently:

names = await NetworkManager.getNetworkInterfaceNames();

Replace that line with:

throw('whatever');

Let me know if that works on SunOS.

@sjorge
Copy link
Contributor Author

sjorge commented Apr 24, 2022

That also seems to work.

Although it looks like the trick with arp is done for FreeBSD and Darwin too so it seems to be the 'common' way CIAO does it for most platforms that are not linux ¯\_(ツ)_/¯

@Supereg Supereg changed the base branch from master to beta-1.1.5 May 20, 2022 07:17
@Supereg Supereg merged commit 0e3e506 into homebridge:beta-1.1.5 May 20, 2022
@sjorge sjorge deleted the sunos branch May 20, 2022 07:33
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.

3 participants