Skip to content

deps: add support for interface broadcast addresses #23438

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

deps: add support for interface broadcast addresses #23438

wants to merge 1 commit into from

Conversation

pprindeville
Copy link

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 12, 2018
@addaleax addaleax added os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version. libuv Issues and PRs related to the libuv dependency or the uv binding. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 12, 2018
@addaleax
Copy link
Member

Hi! Could you open a pull request against https://github.com/libuv/libuv/ with the libuv changes first? We generally avoid doing our own patches on top of libuv releases. (The libuv part also looks semver-major, which may be a challenge on its own.)

@pprindeville pprindeville force-pushed the add-networkInterface-broadcast branch 2 times, most recently from 4e8f315 to fe4e223 Compare October 12, 2018 02:55
@richardlau
Copy link
Member

There are existing discussions in libuv/libuv#158 and libuv/libuv#1371 about extending libuv's support for network interfaces.

@pprindeville
Copy link
Author

pprindeville commented Oct 13, 2018 •
edited
Loading

Hi! Could you open a pull request against https://github.com/libuv/libuv/ with the libuv changes first? We generally avoid doing our own patches on top of libuv releases. (The libuv part also looks semver-major, which may be a challenge on its own.)

Done. See libuv/libuv#2033.

@pprindeville pprindeville force-pushed the add-networkInterface-broadcast branch from fe4e223 to d2111b4 Compare October 13, 2018 20:29
@pprindeville
Copy link
Author

There are existing discussions in libuv/libuv#158 and libuv/libuv#1371 about extending libuv's support for network interfaces.

I saw those, and from what I can tell they deal with larger architectural issues, like getting addresses for non-IP families, interfaces with no layer-3 configuration, etc. That's a bit orthogonal to what I'm doing here.

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Oct 14, 2018
@pprindeville pprindeville force-pushed the add-networkInterface-broadcast branch 2 times, most recently from 8dc06d4 to e219e50 Compare October 19, 2018 00:31
@pprindeville
Copy link
Author

I managed to get this (src/node_os.cc) to compile against libuv/master with my changes, even if there were other files that wouldn't build... was not able to test run, however.

jasnell
jasnell reviewed Oct 22, 2018
View reviewed changes
@pprindeville
Copy link
Author

Currently it returns the empty string

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:20
@gireeshpunathil
Copy link
Member

how do we move forward on this one?

@bnb
Copy link
Contributor

bnb commented Jan 7, 2022

heya @pprindeville! any chance you're going to pick this up again? No worries if not.

If not, anyone else?

If not... we should probably close this out.

@pprindeville
Copy link
Author

heya @pprindeville! any chance you're going to pick this up again? No worries if not.

If not, anyone else?

If not... we should probably close this out.

I can try, but I'm not entirely sure what the blocker is. What do I need to do that I haven't done, or redo, in order to advance this?

@bnb
Copy link
Contributor

bnb commented Jan 11, 2022

@pprindeville it looks like there's an open question (similar to mine!) in libuv/libuv#2033 (review). I'm assuming that if those can get merged in, this can get merged in if I'm reading the PRs correctly. So, ideally, finishing that PR up then coming back to this one once that lands :)

@marco-ippolito
Copy link
Member

@pprindeville I think the PR that was blocking was merged, or I am wrong?

@avivkeller avivkeller added the stalled Issues and PRs that are stalled. label Sep 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@avivkeller
Copy link
Member

@pprindeville This PR hasn't had any activity for a while (and the stalebot isn't up), so I've marked this as stalled. Feel free to continue work, if you still would like to persue this PR.

@pprindeville
Copy link
Author

@pprindeville This PR hasn't had any activity for a while (and the stalebot isn't up), so I've marked this as stalled. Feel free to continue work, if you still would like to persue this PR.

Yeah, after 6 years, whatever expectation of progress I had has dwindled.

@pprindeville pprindeville force-pushed the add-networkInterface-broadcast branch from e219e50 to dd745f2 Compare September 24, 2024 16:20
@pprindeville pprindeville mentioned this pull request Sep 24, 2024
Closed
@pprindeville
os: expose broadcast address in os.networkInterfaces()
840cc89 
Not using the correct broadcast address in applications can cause
disasterous results. Rather than letting the programmer guess at
the broadcast address and try to derive it correctly, allow him to
query the system instead for the correctly configured state.

Fixes: nodejs#23437
Depends-on: libuv/libuv#2033
@pprindeville pprindeville force-pushed the add-networkInterface-broadcast branch from dd745f2 to 840cc89 Compare September 25, 2024 04:33
@github-actions github-actions bot removed the stalled Issues and PRs that are stalled. label Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. libuv Issues and PRs related to the libuv dependency or the uv binding. os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@pprindeville @addaleax @richardlau @gireeshpunathil @bnb @marco-ippolito @avivkeller @jasnell @nodejs-github-bot