add uv_if_indextoname and uv_if_indextoiid #1445

pekkanikander · 2017-07-27T16:32:40Z

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 I don't have
Windows compilation environment. Hence, Windows
help would be appreciated.

This is needed by nodejs/node#14500
This replaces #1443

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

santigimeno · 2017-07-27T16:35:51Z

docs/src/misc.rst

+    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`.
+


A .. versionadded:: tag should be added with a semver-minor jump --> 1.14.0

santigimeno · 2017-07-27T16:37:47Z

src/inet.c

@@ -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);


style: 2-space indentation

santigimeno · 2017-07-27T16:40:45Z

docs/src/misc.rst

+.. 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.


On linux the constant is IF_NAMESIZE so we could add a UV_IF_NAMESIZE constant to avoid platform inconsistencies

santigimeno · 2017-07-27T16:58:17Z

src/inet.c

@@ -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);


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.

@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

Please see https://github.com/libuv/libuv/blob/v1.x/CONTRIBUTING.md#code

@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.

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

pekkanikander · 2017-07-28T16:58:59Z

I pushed a rewritten version. The Windows version has not been tested, since I have no possibility to even compile it.

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

cjihrig

I'm not sure this is the ideal way to organize the code. Left some other comments too.

cjihrig · 2017-07-28T21:37:58Z

include/uv.h

+# define UV_IF_NAMESIZE IF_NAMESIZE
+#elif defined(IFNAMSIZ)
+# define UV_IF_NAMESIZE IFNAMSIZ
+#endif


What happens if neither of these constants are defined for some reason?

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.

cjihrig · 2017-07-28T21:38:33Z

docs/src/misc.rst

+
+    .. versionadded:: 1.14.0
+
+.. c:function:: char* uv_if_indextoname(unsigned int ifindex, char* ifname)


The documented return type seems to be out of sync with the actual return type.

cjihrig · 2017-07-28T21:38:44Z

docs/src/misc.rst

@@ -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


lenght -> length

cjihrig · 2017-07-28T21:40:34Z

docs/src/misc.rst

+
+    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`.


This documentation needs to be updated as well.

I tried to describe the error codes. I could not decipher from the documentation what Windows does in various error conditions.

cjihrig · 2017-07-28T21:47:08Z

src/uv-common.c

+  strncpy(ifname, InterfaceName, UV_IF_NAMESIZE);
+  return 0;
+#else
+  const int saved_errno = errno;


I don't think you need to save, assign, and restore errno.

I tried to be defensive. But of course YMMV.

cjihrig · 2017-07-28T21:47:30Z

src/uv-common.c

+  const int saved_errno = errno;
+  errno = 0;
+  if (NULL == if_indextoname(ifindex, ifname)) {
+    assert(errno);


You shouldn't need this I think.

I agree that if the function returns NULL, errno should be non-zero, but again tried to be defensive.

cjihrig · 2017-07-28T21:49:23Z

src/uv-common.c

+  NET_LUID InterfaceLuid;
+  int error = ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid);
+  if (NO_ERROR != error) {
+    return -error;


Please take a look at how errors are returned from other Windows functions.

I tried to have a look at the other Windows functions. I hope I did the right thing.

saghul

Review round 2.

saghul · 2017-07-30T09:39:41Z

docs/src/misc.rst

+    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


Single underscore. The souble underscore variants are internal.

saghul · 2017-07-30T09:40:00Z

docs/src/misc.rst

+    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


saghul · 2017-07-30T09:40:09Z

docs/src/misc.rst

+    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


heh, famous last works :-)

saghul · 2017-07-30T09:42:25Z

src/uv-common.c

@@ -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) {


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.

saghul · 2017-07-30T09:42:51Z

src/uv-common.c

+  }
+  if (!ConvertInterfaceLuidToAName(
+	InterfaceLuid, InterfaceName, sizeof(InterfaceName)) {
+    return GetLastError();


this doesn't work, you need to convert the windows error into a UV error.

saghul · 2017-07-30T09:44:01Z

include/uv.h

+#elif defined(IFNAMSIZ)
+# define UV_IF_NAMESIZE IFNAMSIZ
+#else
+# warning "Cannot define UV_IF_NAMESIZE: neither IF_NAMESIZE nor IFNAMSIZ defined."


We should define this ourselves to some consdervative value. Just to play safe.

saghul · 2017-07-30T09:46:23Z

src/uv-common.c

+#ifdef _WIN32
+  NET_LUID InterfaceLuid;
+  char InterfaceName[NDIS_IF_MAX_STRING_SIZE + 1];
+  if (!ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid)) {


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.

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.

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

pekkanikander · 2017-08-03T15:55:41Z

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.

santigimeno · 2017-08-04T13:25:59Z

src/unix/getaddrinfo.c

+  char ifname_buf[UV_IF_NAMESIZE];
+  if (NULL == if_indextoname(ifindex, ifname_buf)) {
+    return -errno;
+  }


style: remove braces

Ok, but you guys are looking for trouble...

santigimeno · 2017-08-04T13:26:39Z

src/unix/getaddrinfo.c

+  }
+  {
+    const int required_len = strlen(ifname_buf) + 1;
+    const int ifname_len = *sizep;


style: we usually define local variables at the beginning of the function

Will do, but then I cannot use const. Which I consider bad style. But YMMV.

santigimeno · 2017-08-04T13:26:51Z

src/unix/getaddrinfo.c

+    *sizep = required_len;
+    if (ifname_len < required_len) {
+      return UV_ENAMETOOLONG;
+    }


style: remove braces

santigimeno · 2017-08-04T13:30:17Z

src/unix/getaddrinfo.c

+
+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)) {


shouldn't we set ifname_buf[UV_IF_NAMESIZE - 1] to zero to be sure is NULL terminated?

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.

@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.

santigimeno · 2017-08-04T13:31:59Z

src/unix/getaddrinfo.c

+    if (ifname_len < required_len) {
+      return UV_ENAMETOOLONG;
+    }
+    memcpy(ifname, ifname_buf, required_len);


shouldn't this be strcpy()?

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?

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.

santigimeno

LGTM with some comments

santigimeno · 2017-08-05T08:44:42Z

src/unix/getaddrinfo.c

+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;


style: we usually separate declaration from assignment.

santigimeno · 2017-08-05T08:45:05Z

src/unix/getaddrinfo.c

+  if (NULL == if_indextoname(ifindex, ifname_buf))
+    return -errno;
+
+  ifname_buf[sizeof(ifname_buf)-1] = '\0'; // To be _really_ sure


I think you should drop this as I was wrong,

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.

santigimeno · 2017-08-05T08:49:46Z

src/win/getaddrinfo.c

+  NET_LUID InterfaceLuid;
+  char InterfaceName[NDIS_IF_MAX_STRING_SIZE + 1];
+  int required_len;
+  const int ifname_len = *sizep;


style: separate declaration from assignment

santigimeno · 2017-08-05T08:49:56Z

src/win/getaddrinfo.c

+
+  if (!ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid)) {
+    return uv_translate_sys_error(GetLastError());
+  }


drop braces

santigimeno · 2017-08-05T17:43:40Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/363/

santigimeno · 2017-08-05T17:45:13Z

It looks there are issues on the Windows side:

src\win\getaddrinfo.c(387): error C2065: 'NDIS_IF_MAX_STRING_SIZE': undeclared identifier [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(387): error C2057: expected constant expression [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(387): error C2466: cannot allocate an array of constant size 0 [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(387): error C2133: 'InterfaceName': unknown size [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(391): warning C4013: 'ConvertInterfaceIndexToLuid' undefined; assuming extern returning int [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(394): warning C4013: 'ConvertInterfaceLuidToAName' undefined; assuming extern returning int [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(395): warning C4034: sizeof returns 0 [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(399): error C2065: 'ifname_buf': undeclared identifier [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(399): warning C4034: sizeof returns 0 [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]
src\win\getaddrinfo.c(399): error C2109: subscript requires array or pointer type [c:\workspace\libuv-test-commit-windows\nodes\win2016-vs2015\libuv.vcxproj]

pekkanikander · 2017-08-05T18:04:07Z

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.

saghul · 2017-08-05T19:02:16Z

@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

pekkanikander · 2017-08-06T05:36:11Z

@saghul To be specific, and to give you an understanding of what I don't know:

For ConvertInterfaceIndexToLuid and ConvertInterfaceIndexToLuid, which of the following is correct?

#include <Netioapi.h>
#include <netioapi.h>
#include <Iphlpapi.h>
#include <iphlpapi.h>

And are they really without any prefix path?

saghul · 2017-08-06T09:35:08Z

Number 4 would be correct. No, there is no prefix path needed.

pekkanikander · 2017-08-06T10:51:05Z

The next question is where to place #include <iphlpapi.h>? To the beginning of src/win/getaddrinfo.c? Or src/win/winsock.h? Or somewhere else?

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.

saghul · 2017-08-06T12:43:13Z

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.

pekkanikander · 2017-08-06T12:49:17Z

#include <iphlpapi.h> moved to its hopefully proper place.

santigimeno

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.

santigimeno · 2017-08-06T22:27:35Z

src/win/getaddrinfo.c

+  ifname_len = *sizep;
+
+  if (!ConvertInterfaceIndexToLuid(ifindex, &InterfaceLuid))
+    return uv_translate_sys_error(GetLastError());


It returns zero on success not on error

Sorry I don't understand this comment.

Sorry now I did understand. Fixed.

santigimeno · 2017-08-06T22:28:51Z

src/win/getaddrinfo.c

+    return uv_translate_sys_error(GetLastError());
+
+  if (!ConvertInterfaceLuidToAName(
+      InterfaceLuid, InterfaceName, sizeof(InterfaceName)))


Some issues here:

s/ConvertInterfaceLuidToAName/ConvertInterfaceLuidToNameA

It should be &InterfaceLuid.

It returns zero on success

pekkanikander · 2017-08-14T08:11:45Z

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.

bnoordhuis · 2017-08-14T08:58:52Z

We'll have to wait until someone contributes Windows support.

pekkanikander · 2017-08-14T09:10:47Z

Any idea how to find such someone? All the programmers I know are either Linux or Unix (including Mac OS X) folks.

BridgeAR · 2017-09-19T21:49:24Z

As this currently blocks a PR on Node.js PTAL @santigimeno @saghul @cjihrig.

cjihrig · 2017-09-20T00:59:47Z

src/win/getaddrinfo.c

+  if (r == ERROR_FILE_NOT_FOUND)
+    return UV_ENXIO;
+  else if (r != 0)
+    return UV_EINVAL;


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.

cjihrig · 2017-09-20T01:01:33Z

src/win/getaddrinfo.c

+                                  interface_name,
+                                  sizeof(interface_name));
+  if (r != 0)
+    return UV_EINVAL;


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.

cjihrig · 2017-09-20T01:05:27Z

src/win/getaddrinfo.c

+
+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];


Perhaps a comment that NDIS_IF_MAX_STRING_SIZE does not include the NULL.

cjihrig · 2017-09-20T01:12:21Z

src/win/getaddrinfo.c

+  else if (r != 0)
+    return UV_EINVAL;
+
+  r = ConvertInterfaceLuidToNameA(&interface_luid,


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.

cjihrig · 2017-09-20T01:14:27Z

src/win/getaddrinfo.c

+  if (r != 0)
+    return UV_EINVAL;
+
+  required_len = strnlen(interface_name, sizeof(interface_name)) + 1;


It should be safe to use strlen() here, right?

cjihrig · 2017-09-20T01:26:08Z

src/win/getaddrinfo.c

+  *sizep = required_len;
+
+  if (ifname_len < required_len)
+    return UV_ENOBUFS;


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.

cjihrig · 2017-09-20T01:31:00Z

src/win/getaddrinfo.c

+
+  ifname_len = *sizep;
+
+  *sizep = snprintf(ifname, ifname_len, "%d", ifindex) + 1;


Same comment here about the *sizep value in the two different scenarios.

cjihrig · 2017-09-20T01:33:30Z

src/unix/getaddrinfo.c

@@ -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) {


I think most of the same comments from the Windows code apply here as well.

pekkanikander · 2017-09-20T04:24:41Z

@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 uv_if_indextoiid anyway; I did not understand initially that Windows does not need if_indextoname, as Windows even has a genuine inbuilt implementation if if_indextoname. I was just told not to use it.

Hence, unless someone jumps in and volunteers to take care of the Windows side, I will simply remove the uv_if_indextoname completely, leaving only the (later added) uv_if_indextoiid.

So, which of the following:

Someone volunteers and takes care of the Windows side?
I remove uv_if_indextoname completely?
We let this PR to rotten until the hell freezes?

cjihrig · 2017-09-20T14:19:38Z

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.

pekkanikander · 2017-09-20T14:27:41Z

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 uv_if_indextoiid should be sufficient for the NodeJS PR. I don't see any significant Windows-dependent comments to that side; the couple of minor comments I can probably handle.

So, unless someone else volunteers for taking over (for the Windows), I'll simply remove the uv_if_indextoname. That should take care of the current NodeJS needs.

cjihrig · 2017-09-20T14:31:12Z

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.

pekkanikander · 2017-09-26T12:48:48Z

Any update on this?

cjihrig · 2017-09-26T18:59:24Z

I'll try to spend some time on this tomorrow.

bzoz · 2017-09-27T07:05:35Z

As I understand, we only need uv_if_indextoid for Node PR (both on Linux and Windows). If so, there really is no reason for uv_if_indextoname then, we can drop it.

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

cjihrig · 2017-10-20T19:21:21Z

@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

pekkanikander · 2017-10-30T10:39:33Z

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.

santigimeno

SGTM with a couple of comments

santigimeno · 2017-10-30T10:42:29Z

docs/src/misc.rst

+    Maximum IPv6 interface identifier name length.  Defined as
+    `IFNAMSIZ` on Unix and `IF_NAMESIZE` on Linux and Windows.
+
+    .. versionadded:: 1.15.0


We're now targeting 1.16.0

santigimeno · 2017-10-30T10:50:55Z

src/win/getaddrinfo.c

+  if (buffer == NULL || size == NULL || *size == 0)
+    return UV_EINVAL;
+
+  r = snprintf(buffer, *size, "%d", ifindex) + 1;


Is the + 1 location correct?

This I'd leave for @cjihrig to comment

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.

Changed in f82e7de

santigimeno · 2017-10-30T10:53:06Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/540/

r represents an error or the string length (no NUL) on success, so Colin thinks the rest of the logic is correct.

cjihrig

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.

cjihrig · 2017-11-01T15:30:05Z

@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>

cjihrig · 2017-11-06T14:55:48Z

Landed in 695afe8. Thanks for the contribution and for sticking with it!

pekkanikander · 2017-11-06T15:14:52Z

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>

pekkanikander force-pushed the feature-add-if_indextoname branch from c74a569 to 9196eea Compare July 27, 2017 16:33

santigimeno requested changes Jul 27, 2017

View reviewed changes

santigimeno reviewed Jul 27, 2017

View reviewed changes

pekkanikander force-pushed the feature-add-if_indextoname branch from 9196eea to bc222b3 Compare July 28, 2017 16:58

pekkanikander force-pushed the feature-add-if_indextoname branch from bc222b3 to dc401bf Compare July 28, 2017 17:03

cjihrig requested changes Jul 28, 2017

View reviewed changes

saghul previously requested changes Jul 30, 2017

View reviewed changes

pekkanikander force-pushed the feature-add-if_indextoname branch from 416774f to 42a8788 Compare August 3, 2017 15:52

santigimeno reviewed Aug 4, 2017

View reviewed changes

santigimeno approved these changes Aug 5, 2017

View reviewed changes

santigimeno requested changes Aug 6, 2017

View reviewed changes

cjihrig requested changes Sep 20, 2017

View reviewed changes

pekkanikander force-pushed the feature-add-if_indextoname branch from aac344c to ae9c32c Compare October 30, 2017 10:38

santigimeno approved these changes Oct 30, 2017

View reviewed changes

pekkanikander added 2 commits October 30, 2017 13:17

Minor fix, to be squashed

ddb9593

Dropping + 1 from old meaning of *size.

f82e7de

r represents an error or the string length (no NUL) on success, so Colin thinks the rest of the logic is correct.

cjihrig approved these changes Oct 30, 2017

View reviewed changes

cjihrig mentioned this pull request Nov 3, 2017

1.16.0 Proposal #1617

Closed

cjihrig closed this Nov 6, 2017

cjihrig mentioned this pull request Nov 8, 2017

unix: move net/if.h include #1622

Merged

cjihrig mentioned this pull request Nov 9, 2017

1.16.1 Proposal #1624

Closed


		.. versionadded:: 1.14.0

		.. c:function:: char* uv_if_indextoname(unsigned int ifindex, char* ifname)


		ifname_len = *sizep;

		*sizep = snprintf(ifname, ifname_len, "%d", ifindex) + 1;

add uv_if_indextoname and uv_if_indextoiid #1445

add uv_if_indextoname and uv_if_indextoiid #1445

Conversation

pekkanikander commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pekkanikander commented Jul 28, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pekkanikander commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santigimeno Aug 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santigimeno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santigimeno commented Aug 5, 2017

santigimeno commented Aug 5, 2017

pekkanikander commented Aug 5, 2017

saghul commented Aug 5, 2017

pekkanikander commented Aug 6, 2017

saghul commented Aug 6, 2017

pekkanikander commented Aug 6, 2017

saghul commented Aug 6, 2017

pekkanikander commented Aug 6, 2017

santigimeno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pekkanikander Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pekkanikander commented Aug 14, 2017

bnoordhuis commented Aug 14, 2017

pekkanikander commented Aug 14, 2017

BridgeAR commented Sep 19, 2017

Choose a reason for hiding this comment

santigimeno Aug 4, 2017 •

edited

pekkanikander Aug 7, 2017 •

edited

pekkanikander commented Sep 20, 2017 •

edited