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

Add net_get_interfaces() #2935

Closed
wants to merge 2 commits into from
Closed

Conversation

sgolemon
Copy link
Contributor

New function for enumerating network adapters on the local machine.

This favor's windows' format in the normalized output since it provides more info, and groups addresses on single interfaces together.

Output example for windows and linux here:
https://twitter.com/SaraMG/status/933821878510211072

# include <iphlpapi.h>
#endif

zend_string* php_inet_ntop(const struct sockaddr *addr) {
Copy link
Member

Choose a reason for hiding this comment

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

This is missing PHPAPI as you have declared in the header

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally the first declaration is decisive, but would be cleaner to repeat same, yep.

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @weltling stated, not needed at point of definition when the included declaration has it, but sure, hurts nothing to add it.

dwRetVal = GetAdaptersAddresses(family, flags, NULL, pAddresses, &outBufLen);

if (NO_ERROR != dwRetVal) {
/* TODO check GetLastError() */
Copy link
Member

Choose a reason for hiding this comment

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

Think this is missing FREE(pAddresses) before the return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@KalleZ
Copy link
Member

KalleZ commented Nov 24, 2017

(For any other readers)

This implements FR #17400: https://bugs.php.net/bug.php?id=17400

@@ -22,13 +22,15 @@ AC_DEFINE("PHP_USE_PHP_CRYPT_R", 1);

CHECK_HEADER_ADD_INCLUDE("timelib_config.h", "CFLAGS_STANDARD", "ext/date/lib");

ADD_FLAG("LIBS_STANDARD", "iphlpapi.lib");
Copy link
Member

Choose a reason for hiding this comment

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

@weltling tho we only use iphlpapi here, wouldn't you think its better put in toolset_setup_common_libs() in win32/confutils.js, ext/standard can't be disabled anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

@KalleZ would work as well, but shouldn't be necessary. $(LIBS) is put for every binary unit, whereby iphlpapi.lib is only required for standard. The linker will ignore it anyway, if there are no symbols used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds like six of one, half a dozen of the other. I honestly don't mind either way, but I read @weltling's comment (and @KalleZ's 👍) as "don't bother moving it. Correct me if that's wrong.

PIP_ADAPTER_UNICAST_ADDRESS u = NULL;
ULONG outBufLen = 0;
DWORD dwRetVal = 0;

Copy link
Member

Choose a reason for hiding this comment

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

This should have a ZEND_PARSE_PARAMETERS_NONE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.

@winks
Copy link
Contributor

winks commented Nov 26, 2017

Per my comments on internals, just tested this on FreeBSD 11.1-RELEASE (after a plain ./configure)

Warning: Use of undefined constant AF_INET - assumed 'AF_INET' (this will throw an Error in a future version of PHP) in /tmp/test.php on line 11
bool(false)
array(2) {
  ["vtnet0"]=>
  array(1) {
    ["unicast"]=>
    array(4) {
      [0]=>
      array(2) {
        ["flags"]=>
        int(34883)
        ["family"]=>
        int(18)
      }
      [1]=>
      array(5) {
        ["flags"]=>
        int(34883)
        ["family"]=>
        int(2)
        ["address"]=>
        string(15) "207.154.243.135"
        ["netmask"]=>
        string(13) "255.255.240.0"
        ["broadcast"]=>
        string(15) "207.154.255.255"
      }
      [2]=>
      array(4) {
        ["flags"]=>
        int(34883)
        ["family"]=>
        int(28)
        ["address"]=>
        string(25) "fe80::2c1c:f1ff:fec7:8400"
        ["netmask"]=>
        string(21) "ffff:ffff:ffff:ffff::"
      }
      [3]=>
      array(5) {
        ["flags"]=>
        int(34883)
        ["family"]=>
        int(2)
        ["address"]=>
        string(9) "10.19.0.5"
        ["netmask"]=>
        string(11) "255.255.0.0"
        ["broadcast"]=>
        string(13) "10.19.255.255"
      }
    }
  }
  ["lo0"]=>
  array(1) {
    ["unicast"]=>
    array(4) {
      [0]=>
      array(2) {
        ["flags"]=>
        int(32841)
        ["family"]=>
        int(18)
      }
      [1]=>
      array(4) {
        ["flags"]=>
        int(32841)
        ["family"]=>
        int(28)
        ["address"]=>
        string(3) "::1"
        ["netmask"]=>
        string(39) "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"
      }
      [2]=>
      array(4) {
        ["flags"]=>
        int(32841)
        ["family"]=>
        int(28)
        ["address"]=>
        string(7) "fe80::1"
        ["netmask"]=>
        string(21) "ffff:ffff:ffff:ffff::"
      }
      [3]=>
      array(4) {
        ["flags"]=>
        int(32841)
        ["family"]=>
        int(2)
        ["address"]=>
        string(9) "127.0.0.1"
        ["netmask"]=>
        string(9) "255.0.0.0"
      }
    }
  }
}

```

@sgolemon
Copy link
Contributor Author

@winks Ah, hrmm. Well, that's the implementation working, but the unittest failing because AF_INET is defined by the sockets extension which isn't always enabled. Nice catch.

@sgolemon
Copy link
Contributor Author

Committed to 7.3 as 7ca5a7d

@sgolemon sgolemon closed this Nov 27, 2017
@KalleZ
Copy link
Member

KalleZ commented Nov 27, 2017

@sgolemon we do have STREAM_PF_INET which is either PF_INET or AF_INET depending on what constant was available at compile time in the core

@remicollet
Copy link
Member

@sgolemon woulnd't it makes sense to expose a few constants from if.h to be able to analyse the "flags".

Can even be nice to be able to filter according to it:
$interfaces = net_get_interfaces(IFF_UP & IFF_RUNNING);
(even if this can be trivially done in php lang)

@remicollet
Copy link
Member

so pr #2948

@cubiclesoft
Copy link

This function is buggy, undocumented, and several opportunities were missed. See:

https://bugs.php.net/bug.php?id=79251

I see that no one noticed the obvious "unicast" array bug during the initial implementation despite one sample output above clearly showing the problem on this PR's thread. That is, AF_PACKET (17) and AF_LINK (18) families have MAC addresses, not IP addresses. As a result, the first entry in each interface's "unicast" array is missing its associated information.

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

Successfully merging this pull request may close these issues.

6 participants