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

include, misc: Add net interface API. #1371

Closed
wants to merge 1 commit into from

Conversation

apaprocki
Copy link
Contributor

Building on the work done by @zenoamaro, per +1'd approach laid out in #219, this PR lands the new interface without modifying any existing code to depend on it. All platforms return -ENOTSUP initially and the test driver skips the test if -ENOTSUP is returned. Once all platforms have been implemented in subsequent PRs, the test will change to be required and the documentation note will be removed and existing code such as uv_interface_addresses can then change to rely on the new API.

CC: @bnoordhuis @saghul

@apaprocki
Copy link
Contributor Author

@bnoordhuis Should anyone else be pulled in on this? Or is it okay to move forward with this and start opening up PRs for the individual platform support?

@zenoamaro
Copy link

I am ok with this. In retrospect, it should have been this way from the beginning 👍

@saghul
Copy link
Member

saghul commented Jun 9, 2017

I think I'd rather have it for v2, that is, targeted at master. If this is to replace the old API we can remove it there.

WDYT?

@bnoordhuis
Copy link
Member

It's useful enough IMO that I'd be okay with targeting v1. I'll review the code.

docs/src/misc.rst Outdated Show resolved Hide resolved
test/test-platform-output.c Outdated Show resolved Hide resolved
test/test-platform-output.c Outdated Show resolved Hide resolved
@bnoordhuis
Copy link
Member

@apaprocki @zenoamaro Are you still interested in pursuing this?

docs/src/misc.rst Outdated Show resolved Hide resolved
@apaprocki
Copy link
Contributor Author

@bnoordhuis Yes, just haven't had the time to loopback (ha ha) to this.

@cwalther
Copy link

If the plan is still to eventually implement uv_interface_addresses() in terms of the new uv_network_interfaces(), then I would like to hear your opinion on nodejs/node#14977, because if it is considered worth including, the required additional fields could be added to the uv_network_interface_t struct before it is declared immutable ABI. (There is still the ABI break of adding them to uv_interface_address_t, but I would rather have one ABI break than two.)

@lundibundi
Copy link

@apaprocki what's the status of this, do you plan to continue this PR?

Copy link

@pprindeville pprindeville left a comment

Choose a reason for hiding this comment

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

Don't know if this PR is still in play or not (after 18 months), but here are some suggestions...

include/uv.h Outdated Show resolved Hide resolved


void uv_free_network_interfaces(uv_network_interface_t* interfaces, int count) {
int i;

Choose a reason for hiding this comment

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

Given that the domain of i is positive numbers, I'd make this an unsigned here and elsewhere below.

I'd do the same for count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pprindeville I preserved the same API semantics as the other existing platform APIs. This also allows the new functions to plug right into the existing test driver without declaring a new variable. I'm open to changing this, but not sure why this one API should differ from the others.

include/uv.h Outdated Show resolved Hide resolved
@ryzokuken
Copy link
Contributor

Closing the PR because it seems to be stalled. Feel free to reopen if anyone wants to continue working on this.

@ryzokuken
Copy link
Contributor

Oops, this is libuv, and I'm not a collaborator 😅 Weird how we take things for granted.

@cjihrig help?

@apaprocki
Copy link
Contributor Author

@bnoordhuis I had time to revisit this and I changed a lot of it...

  • Changed the struct to use a bitmask as you suggested, with macro accessors for the flags
  • Support up to 16-byte physical addresses and dynamically print out whatever is reported
  • Stop storing netmask values directly and instead compute the CIDR prefix and only render it to a netmask address for IPv4 addresses

A lot of this is still opinionated, so please re-review and let me know what direction it should take. I test implemented the BSD version of this on my Mac and verified it works as expected:

e.g.

#   name: lo0
#     running: 1
#     is_loopback: 1
#     is_point_to_point: 0
#     is_promiscuous: 0
#     has_broadcast: 0
#     has_multicast: 1
#     address: inet6 ::1/128
#   name: lo0
#     running: 1
#     is_loopback: 1
#     is_point_to_point: 0
#     is_promiscuous: 0
#     has_broadcast: 0
#     has_multicast: 1
#     address: inet6 fe80::1/64
#   name: en0
#     running: 1
#     is_loopback: 0
#     is_point_to_point: 0
#     is_promiscuous: 0
#     has_broadcast: 1
#     has_multicast: 1
#     address: inet6 fe80::16:900f:XXXX:XXXX/64
#     physical address: XX:XX:XX:XX:XX:XX
#   name: en0
#     running: 1
#     is_loopback: 0
#     is_point_to_point: 0
#     is_promiscuous: 0
#     has_broadcast: 1
#     has_multicast: 1
#     address: inet4 192.168.X.X
#     netmask: inet4 255.255.255.255
#     physical address: XX:XX:XX:XX:XX:XX
#   name: utun0
#     running: 1
#     is_loopback: 0
#     is_point_to_point: 1
#     is_promiscuous: 0
#     has_broadcast: 0
#     has_multicast: 1
#     address: inet6 fe80::8fcc:XXXX:XXXX:XXXX/64

Adds `uv_network_interface_t` structure and a new API for retrieving
network interfaces, `uv_network_interfaces`.  This new API intends to
eventually replace `uv_interface_addresses`, adding new capabilities,
such as:

 * Not filtering out interfaces without addresses;
 * Marking missing addresses as `AF_UNSPEC`;
 * Including all common cross-platform flags;
 * Including link-layer addresses for interfaces;
 * Supporting 8-byte Firewire link-layer addresses;
 * Including broadcast/point-to-point addresses

The interface is added and documented, and returns `UV_ENOTSUP` on
all platforms.  Once the individual platform support is merged, the
test driver will no longer accept the `UV_ENOTSUP` return and the
existing `uv_interface_addresses` function will be changed to be
implemented on top of this new API.
@apaprocki
Copy link
Contributor Author

@bnoordhuis I rebased this again. I think if there's actually a v2 in the works, it would be good to include the new API if this is desired.

@cwalther
Copy link

cwalther commented Sep 3, 2019

@pprindeville
Copy link

Is there any progress on this?

@stale
Copy link

stale bot commented Dec 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

1 similar comment
@stale
Copy link

stale bot commented Dec 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 13, 2019
@stale stale bot closed this Dec 20, 2019
@ShogunPanda
Copy link

Hello! In Node.js we'd like to have this in order to close nodejs/node#498.
Do you have any plan to implement this? Do you need any help?

@piranna
Copy link

piranna commented Mar 11, 2024

How can we get this move forward? This has been blocked on Node.js since more than 5 years ago...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet