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
add uv_if_indextoname and uv_if_indextoiid #1445
Conversation
c74a569
to
9196eea
Compare
if_indextoname(...) is a function defined in <net/if.h> It is used to convert an IPv6 scope_id into an interface identifier string such as %eth0 or %lo There is an identical function in Windows, but that has not been tested. Adds documentation and an ASSERT to the relevant test case. PR-URL: libuv#1445
docs/src/misc.rst
Outdated
Cross-platform IPv6-capable implementation of :man:`if_indextoname(3)`. | ||
Writes the result to `ifname`, which must be at least `IFNAMSIZ` long. | ||
On success returns the generated string. In case of error returns `NULL`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A .. versionadded::
tag should be added with a semver-minor jump --> 1.14.0
src/inet.c
Outdated
@@ -49,6 +49,9 @@ int uv_inet_ntop(int af, const void* src, char* dst, size_t size) { | |||
/* NOTREACHED */ | |||
} | |||
|
|||
char* uv_if_indextoname(unsigned int ifindex, char* ifname) { | |||
return if_indextoname(ifindex, ifname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: 2-space indentation
docs/src/misc.rst
Outdated
.. c:function:: char* uv_if_indextoname(unsigned int ifindex, char* ifname) | ||
|
||
Cross-platform IPv6-capable implementation of :man:`if_indextoname(3)`. | ||
Writes the result to `ifname`, which must be at least `IFNAMSIZ` long. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On linux the constant is IF_NAMESIZE
so we could add a UV_IF_NAMESIZE
constant to avoid platform inconsistencies
src/inet.c
Outdated
@@ -49,6 +49,9 @@ int uv_inet_ntop(int af, const void* src, char* dst, size_t size) { | |||
/* NOTREACHED */ | |||
} | |||
|
|||
char* uv_if_indextoname(unsigned int ifindex, char* ifname) { | |||
return if_indextoname(ifindex, ifname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I would probably change the signature of the function so it returns an int
so it returns 0 on success or -errno
on error.
On Windows apparently if if_indextoname
returns NULL there's no way to check the error, so it's probably better use this other method explained in the documentation:
The if_indextoname function is implemented for portability of applications with Unix environments, but the ConvertInterface functions are preferred. The if_indextoname function can be replaced by a call to the ConvertInterfaceIndexToLuid function to convert an interface index to a NET_LUID followed by a call to the ConvertInterfaceLuidToNameA to convert the NET_LUID to the ANSI interface name.
If the if_indextoname fails and returns a NULL pointer, it is not possible to determine an error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@santigimeno I can do this (at least the first version), but I have difficulty in figuring out in which file(s) I should place the Unix and Windows versions. Could you please suggest suitable source files for the source code?
--Pekka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saghul and @santigimeno I have read the contribution instructions a few times. The problem is that some of the related functions are already in src/inet.c
, and some of them are in src/uv-common.c
. None of them are so far in src/unix/
or src/win
. Hence, in src/unix/
or src/win
there are basically no existing files where I could add this function. I would not think the right choice is to create yet another source file for just this simple function.
So, since there seems to be already some #ifdef _WIN32
conditionals in src/uv-common.c
, and since all Unix variants (including Linux) should have the same underlying function signature anyway, I'm now moving the source code from src/inet.c
(where it would belong IMHO) to src/uv-common.c
, with the actual implementation behind #ifdef _WIN32
. Personally, I find the solution ugly due to the new #ifdef
, but I couldn't find any better place.
9196eea
to
bc222b3
Compare
if_indextoname(...) is a function defined in <net/if.h> It is used to convert an IPv6 scope_id into an interface identifier string such as %eth0 or %lo The Windows implementation has not been tested. Adds documentation and an ASSERT to the relevant test case. PR-URL: libuv#1445
I pushed a rewritten version. The Windows version has not been tested, since I have no possibility to even compile it. |
bc222b3
to
dc401bf
Compare
if_indextoname(...) is a function defined in <net/if.h> It is used to convert an IPv6 scope_id into an interface identifier string such as %eth0 or %lo The Windows implementation has not been tested. Adds documentation and an ASSERT to the relevant test case. PR-URL: libuv#1445
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the ideal way to organize the code. Left some other comments too.
include/uv.h
Outdated
# define UV_IF_NAMESIZE IF_NAMESIZE | ||
#elif defined(IFNAMSIZ) | ||
# define UV_IF_NAMESIZE IFNAMSIZ | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if neither of these constants are defined for some reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If neither of them are defined, then presumably the underlying OS does not support if_indextoname
either. Should that invoke an #error
or #warning
?
For the next version I'll place there a #warning
, but of course that can be removed, if needed.
docs/src/misc.rst
Outdated
|
||
.. versionadded:: 1.14.0 | ||
|
||
.. c:function:: char* uv_if_indextoname(unsigned int ifindex, char* ifname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documented return type seems to be out of sync with the actual return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
docs/src/misc.rst
Outdated
@@ -271,6 +271,21 @@ API | |||
and :man:`inet_pton(3)`. On success they return 0. In case of error | |||
the target `dst` pointer is unmodified. | |||
|
|||
.. c:macro:: UV_IF_NAMESIZE | |||
|
|||
Maximum IPv6 interface identifier name lenght. Defined as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lenght -> length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
docs/src/misc.rst
Outdated
|
||
Cross-platform IPv6-capable implementation of :man:`if_indextoname(3)`. | ||
Writes the result to `ifname`, which must be at least `UV_IF_NAMESIZE` long. | ||
On success returns the generated string. In case of error returns `NULL`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation needs to be updated as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to describe the error codes. I could not decipher from the documentation what Windows does in various error conditions.
src/uv-common.c
Outdated
strncpy(ifname, InterfaceName, UV_IF_NAMESIZE); | ||
return 0; | ||
#else | ||
const int saved_errno = errno; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to save, assign, and restore errno
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to be defensive. But of course YMMV.
src/uv-common.c
Outdated
const int saved_errno = errno; | ||
errno = 0; | ||
if (NULL == if_indextoname(ifindex, ifname)) { | ||
assert(errno); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need this I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that if the function returns NULL
, errno
should be non-zero, but again tried to be defensive.
src/uv-common.c
Outdated
NET_LUID InterfaceLuid; | ||
int error = ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid); | ||
if (NO_ERROR != error) { | ||
return -error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at how errors are returned from other Windows functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to have a look at the other Windows functions. I hope I did the right thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review round 2.
docs/src/misc.rst
Outdated
Cross-platform IPv6-capable implementation of :man:`if_indextoname(3)`. | ||
Writes the result to `ifname`, which must be at least `UV_IF_NAMESIZE` | ||
long. On success, returns zero. If the interface is not found, | ||
returns `UV__ENXIO`. If the interface name is longer than what the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single underscore. The souble underscore variants are internal.
docs/src/misc.rst
Outdated
Writes the result to `ifname`, which must be at least `UV_IF_NAMESIZE` | ||
long. On success, returns zero. If the interface is not found, | ||
returns `UV__ENXIO`. If the interface name is longer than what the | ||
function can handle, returns `UV__ENAMETOOLONG` (Windows only. Should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
docs/src/misc.rst
Outdated
long. On success, returns zero. If the interface is not found, | ||
returns `UV__ENXIO`. If the interface name is longer than what the | ||
function can handle, returns `UV__ENAMETOOLONG` (Windows only. Should | ||
not happen ever.) In Windows, may also return other error values under |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, famous last works :-)
src/uv-common.c
Outdated
@@ -225,6 +225,30 @@ int uv_ip6_name(const struct sockaddr_in6* src, char* dst, size_t size) { | |||
return uv_inet_ntop(AF_INET6, &src->sin6_addr, dst, size); | |||
} | |||
|
|||
int uv_if_indextoname(unsigned int ifindex, char* ifname) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other functions we have which write to a buffer take a second length parameter. I'd like us to do the same here. It would be an in-out parameter. See this similar API: http://docs.libuv.org/en/v1.x/fs_poll.html#c.uv_fs_poll_getpath
Please make sure the terminating NULL byte is handled consistently.
src/uv-common.c
Outdated
} | ||
if (!ConvertInterfaceLuidToAName( | ||
InterfaceLuid, InterfaceName, sizeof(InterfaceName)) { | ||
return GetLastError(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't work, you need to convert the windows error into a UV error.
include/uv.h
Outdated
#elif defined(IFNAMSIZ) | ||
# define UV_IF_NAMESIZE IFNAMSIZ | ||
#else | ||
# warning "Cannot define UV_IF_NAMESIZE: neither IF_NAMESIZE nor IFNAMSIZ defined." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should define this ourselves to some consdervative value. Just to play safe.
src/uv-common.c
Outdated
#ifdef _WIN32 | ||
NET_LUID InterfaceLuid; | ||
char InterfaceName[NDIS_IF_MAX_STRING_SIZE + 1]; | ||
if (!ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having Windows APIs in uv-common, let's split this and have it in src/unix/core.c and src/win/util.c please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to split into src/unix/getaddrinfo.c
and src/win/getaddrinfo.c
instead, since the interface names are integral parts of IPv6 link local addresses.
416774f
to
42a8788
Compare
if_indextoname(...) is a function defined in <net/if.h> It is used to convert an IPv6 scope_id into an interface identifier string such as %eth0 or %lo The Windows implementation has not been tested. Adds documentation and an ASSERT to the relevant test case. PR-URL: libuv#1445
I tried to address all review round 2 comments. The Windows version is still completely untested, since I have no possibility of even compiling it. |
src/unix/getaddrinfo.c
Outdated
char ifname_buf[UV_IF_NAMESIZE]; | ||
if (NULL == if_indextoname(ifindex, ifname_buf)) { | ||
return -errno; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: remove braces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but you guys are looking for trouble...
src/unix/getaddrinfo.c
Outdated
} | ||
{ | ||
const int required_len = strlen(ifname_buf) + 1; | ||
const int ifname_len = *sizep; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: we usually define local variables at the beginning of the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, but then I cannot use const
. Which I consider bad style. But YMMV.
src/unix/getaddrinfo.c
Outdated
*sizep = required_len; | ||
if (ifname_len < required_len) { | ||
return UV_ENAMETOOLONG; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: remove braces
src/unix/getaddrinfo.c
Outdated
|
||
int uv_if_indextoname(unsigned int ifindex, char* ifname, size_t *sizep) { | ||
char ifname_buf[UV_IF_NAMESIZE]; | ||
if (NULL == if_indextoname(ifindex, ifname_buf)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we set ifname_buf[UV_IF_NAMESIZE - 1]
to zero to be sure is NULL terminated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is needed, the underlying implementation should never return anything that is even close to IFNAMSIZ in length. But I can add such an assignment if you want to have both a belt and suspenders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pekkanikander you're right, I was overthinking. I was looking into the musl implementation and saw it was returning the result of strncpy
and thought that it could happen that the buffer would end up not NULL terminated in some cases. But that's not possible as ifreq.ifr_name
is of size IFNAMSIZ
. Sorry for the noise.
src/unix/getaddrinfo.c
Outdated
if (ifname_len < required_len) { | ||
return UV_ENAMETOOLONG; | ||
} | ||
memcpy(ifname, ifname_buf, required_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be strcpy()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? strcpy
is unsafe. I could use strncpy
, but since we already know the length of the name, why to use strncpy
which is less efficient that memcpy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case strcpy
should be safe as we know ifname
is least of the size of ifname_buf
but you're right memcpy
should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some comments
src/unix/getaddrinfo.c
Outdated
int uv_if_indextoname(unsigned int ifindex, char* ifname, size_t *sizep) { | ||
char ifname_buf[UV_IF_NAMESIZE]; | ||
int required_len; | ||
const int ifname_len = *sizep; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: we usually separate declaration from assignment.
src/unix/getaddrinfo.c
Outdated
if (NULL == if_indextoname(ifindex, ifname_buf)) | ||
return -errno; | ||
|
||
ifname_buf[sizeof(ifname_buf)-1] = '\0'; // To be _really_ sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should drop this as I was wrong,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped the separate assignment, but also changed strlen
to strnlen
to avoid running over the buffer even in the case there was some obscure internal hiccup.
src/win/getaddrinfo.c
Outdated
NET_LUID InterfaceLuid; | ||
char InterfaceName[NDIS_IF_MAX_STRING_SIZE + 1]; | ||
int required_len; | ||
const int ifname_len = *sizep; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: separate declaration from assignment
src/win/getaddrinfo.c
Outdated
|
||
if (!ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid)) { | ||
return uv_translate_sys_error(GetLastError()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop braces
It looks there are issues on the Windows side:
|
Unfortunately I have no ability to address the Windows issues. As I have mentioned a couple of times, I have last time used Windows back in 2001, and never developed any code on Windows. Hence, I am not able to even guess what should be included or what else could be wrong. I will address the remaining style comments either later today or tomorrow. |
@pekkanikander Microsoft's documentation is pretty good. Searching for the function names will yield its documentation page, which mentions which file needs to be included. As an example: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365826(v=vs.85).aspx |
@saghul To be specific, and to give you an understanding of what I don't know: For
And are they really without any prefix path? |
Number 4 would be correct. No, there is no prefix path needed. |
The next question is where to place Now it is there, but in a wrong place. But this might compile under Windows; it would be nice to see what compilation errors there are now on Windows. |
You can place it in getaddrinfo.c, where it's actually used. Just like another import file, there is nothing special about it, so at the top. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments after testing this on Windows
. Still, the test fails as the device_name
does not match the scoped_addr
. On my VM is Ethernet
vs ethernet_32768
. Haven't investigated it though.
src/win/getaddrinfo.c
Outdated
ifname_len = *sizep; | ||
|
||
if (!ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid)) | ||
return uv_translate_sys_error(GetLastError()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns zero
on success not on error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't understand this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry now I did understand. Fixed.
src/win/getaddrinfo.c
Outdated
return uv_translate_sys_error(GetLastError()); | ||
|
||
if (!ConvertInterfaceLuidToAName( | ||
InterfaceLuid, InterfaceName, sizeof(InterfaceName))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some issues here:
- s/ConvertInterfaceLuidToAName/ConvertInterfaceLuidToNameA
- It should be
&InterfaceLuid
. - It returns
zero
on success
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
What happens to this next? As far as I understand, it is now the Windows side which is holding this back. And as I have written, I don't know Windows at all. So, any suggestions on how to proceed? This is needed for making NodeJS to handle IPv6 link local packets correctly with UDP. |
We'll have to wait until someone contributes Windows support. |
Any idea how to find such someone? All the programmers I know are either Linux or Unix (including Mac OS X) folks. |
As this currently blocks a PR on Node.js PTAL @santigimeno @saghul @cjihrig. |
src/win/getaddrinfo.c
Outdated
if (r == ERROR_FILE_NOT_FOUND) | ||
return UV_ENXIO; | ||
else if (r != 0) | ||
return UV_EINVAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though there is only one other error besides ERROR_FILE_NOT_FOUND
, perhaps this should call uv_translate_sys_error()
to be a little less confusing.
src/win/getaddrinfo.c
Outdated
interface_name, | ||
sizeof(interface_name)); | ||
if (r != 0) | ||
return UV_EINVAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use uv_translate_sys_error()
I think. One of the errors returned by ConvertInterfaceLuidToNameA()
is ERROR_NOT_ENOUGH_MEMORY
, which doesn't really correspond to UV_EINVAL
.
src/win/getaddrinfo.c
Outdated
|
||
int uv_if_indextoname(unsigned int ifindex, char* ifname, size_t* sizep) { | ||
NET_LUID interface_luid; | ||
char interface_name[NDIS_IF_MAX_STRING_SIZE + 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a comment that NDIS_IF_MAX_STRING_SIZE
does not include the NULL.
src/win/getaddrinfo.c
Outdated
else if (r != 0) | ||
return UV_EINVAL; | ||
|
||
r = ConvertInterfaceLuidToNameA(&interface_luid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to use ConvertInterfaceLuidToNameA()
instead of ConvertInterfaceLuidToNameW()
? My experience has been that on Windows, libuv uses the W
variants of these functions, and then converts them.
src/win/getaddrinfo.c
Outdated
if (r != 0) | ||
return UV_EINVAL; | ||
|
||
required_len = strnlen(interface_name, sizeof(interface_name)) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be safe to use strlen()
here, right?
src/win/getaddrinfo.c
Outdated
*sizep = required_len; | ||
|
||
if (ifname_len < required_len) | ||
return UV_ENOBUFS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the success case, we should be returning a string length in *sizep
. In the case of UV_ENOBUFS
, it should be a size (length + 1). Right now, the UV_ENOBUFS
case is correct, but the success case is not.
src/win/getaddrinfo.c
Outdated
|
||
ifname_len = *sizep; | ||
|
||
*sizep = snprintf(ifname, ifname_len, "%d", ifindex) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here about the *sizep
value in the two different scenarios.
src/unix/getaddrinfo.c
Outdated
@@ -200,3 +200,28 @@ void uv_freeaddrinfo(struct addrinfo* ai) { | |||
if (ai) | |||
freeaddrinfo(ai); | |||
} | |||
|
|||
|
|||
int uv_if_indextoname(unsigned int ifindex, char* ifname, size_t* sizep) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of the same comments from the Windows code apply here as well.
@cjihrig @refack @bzoz and all: As I have written above, I have no Windows experience. Given my schedule and the my previous struggling with the changes requested on the Windows version, I have no option but to refrain from doing any more work on the Windows version. Last time it took several hours for getting such "small" changes right, and then another one of you came and basically rewrote the whole Windows version. The NodeJS PR requires only the Hence, unless someone jumps in and volunteers to take care of the Windows side, I will simply remove the So, which of the following:
|
If you're unwilling or unable to implement the Windows pieces, then someone else should probably take over the PR. Unfortunately, the Windows side needs to work as well before we can accept this. |
I am unable to make any reasonable changes to the Windows pieces, since I cannot compile nor test it. I haven't much touched Windows since 2001 or so; I am basically unable to use modern Windows versions (i.e extremely clumsy due to the UI changes). Windows XP is the latest version I am somewhat comfortable with, but I don't have it installed anywhere any more. As I wrote, the So, unless someone else volunteers for taking over (for the Windows), I'll simply remove the |
My Windows setup isn't great either. I use Windows 7 inside of VirtualBox on my mac. For the purposes of working on libuv, you can get away with pretty much only using the command line. It's a bit of a pain to get setup, but then you can at least compile and run the test suite locally. |
Any update on this? |
I'll try to spend some time on this tomorrow. |
As I understand, we only need In any case I can pickup the Windows side of this if needed. |
if_indextoname(...) is a function defined in <net/if.h> It is used to convert an IPv6 scope_id into an interface identifier string such as %eth0 or %lo uv_if_indextoiid is a cross-platform implementation that returns an IPv6 interface identifier. On Unix/Linux it calls if_indextoname, on Windows it uses snprintf to return the numeric interface identifier as a string. Adds documentation and an ASSERT to the relevant test case. PR-URL: libuv#1445
@pekkanikander sorry it took longer than I had expected to get to this. I reworked this PR in cjihrig@f939aa6. Feel free to use that however you want. The corresponding CI run is https://ci.nodejs.org/view/libuv/job/libuv-test-commit/534/. |
if_indextoname(...) is a function defined in <net/if.h> It is used to convert an IPv6 scope_id into an interface identifier string such as %eth0 or %lo uv_if_indextoiid is a cross-platform implementation that returns an IPv6 interface identifier. On Unix/Linux it calls if_indextoname, on Windows it uses snprintf to return the numeric interface identifier as a string. Adds documentation and an ASSERT to the relevant test case. PR-URL: libuv#1445 Reworked based on f939aa6
aac344c
to
ae9c32c
Compare
This is now a version reworked based on Colin's version. Thanks Colin! The Windows version is directly taken from Colin's work. The documentation and Unix version I double checked, trying to fix even the tiny nits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM with a couple of comments
docs/src/misc.rst
Outdated
Maximum IPv6 interface identifier name length. Defined as | ||
`IFNAMSIZ` on Unix and `IF_NAMESIZE` on Linux and Windows. | ||
|
||
.. versionadded:: 1.15.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now targeting 1.16.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
src/win/getaddrinfo.c
Outdated
if (buffer == NULL || size == NULL || *size == 0) | ||
return UV_EINVAL; | ||
|
||
r = snprintf(buffer, *size, "%d", ifindex) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the + 1
location correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I'd leave for @cjihrig to comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the + 1
should be dropped. r
represents an error or the string length (no NULL) on success, so I think the rest of the logic is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in f82e7de
r represents an error or the string length (no NUL) on success, so Colin thinks the rest of the logic is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I can review this, but LGTM
Dismissing as it looks like everything has been addressed, and the PR has changed quite a bit since.
@saghul I dismissed your old review since the PR has changed since then, and it appears that the objections had been addressed. Would you care to take another look at this? |
uv_if_indextoname() is used to convert an IPv6 scope_id to an interface identifier string such as %eth0 or %lo. uv_if_indextoiid() returns an IPv6 interface identifier. On Unix it calls uv_if_indextoname(). On Windows it uses snprintf() to return the numeric interface identifier as a string. Refs: nodejs/node#14500 PR-URL: #1445 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in 695afe8. Thanks for the contribution and for sticking with it! |
Thank you @cjihrig and all! I'll proceed with nodejs/node#14500 on this Wednesday; don't have time tomorrow. |
This commit moves the net/if.h include into src/getaddrinfo.c to prevent AIX compilation errors. With these symbols exposed publicly, Node.js compilation failed on AIX by exposing Free(), which conflicts with another API. Refs: nodejs/node#16835 Refs: libuv#1445 PR-URL: libuv#1622 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
NDIS_IF_MAX_STRING_SIZE does not appear to be available on some Windows systems. This commit defines it using the same logic used by Wireshark. See: https://github.com/boundary/wireshark/blob/07eade8124fd1d5386161591b52e177ee6ea849f/capture_win_ifnames.c#L42-L44 Refs: nodejs/node#16835 Refs: #1445 PR-URL: #1623 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
if_indextoname(...)
is a function defined in <net/if.h>It is used to convert an IPv6
scope_id
into an interfaceidentifier string such as
%eth0
or%lo
There is an identical function in Windows, but I don't have
Windows compilation environment. Hence, Windows
help would be appreciated.
This is needed by nodejs/node#14500
This replaces #1443