homebridge / ciao

RFC 6762 and RFC 6763 compliant mdns service discovery library written in Typescript
MIT License
79 stars 6 forks source link

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

Closed sjorge closed 2 years ago

sjorge commented 2 years ago

:recycle: 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.

:bulb: 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.

:gear: Release Notes

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

:heavy_plus_sign: 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.

adriancable commented 2 years ago

@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 commented 2 years ago

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

adriancable commented 2 years ago

@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 commented 2 years ago

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 ¯\_(ツ)_/¯