net_get_interfaces()

6 years ago by Anatol Belski — view source

unread

-----Original Message-----
From: php@golemon.com [mailto:php@golemon.com] On Behalf Of Sara
Golemon
Sent: Thursday, November 23, 2017 11:46 PM
To: PHP internals internals@lists.php.net
Subject: [PHP-DEV] net_get_interfaces()

Planning to add net_get_interfaces()
https://github.com/php/php-src/pull/2935/files for enumerating the adapters
on a system and their configured addresses.
Based on a combination of my own implementation and finding out krakjoe and
ab@ had a stalled version already.

If anyone wants an RFC I'll write one up, but this is pretty small so mostly just
offering for a look before I push.

There's no such functionality in the core till now, whereby it was asked some years ago. IMHO it would make a little sense with the RFC, until perhaps some would doubt such a functionality makes sense.

Thanks for moving forward on this, Sara ?

Regards

anatol

6 years ago by Florian Anderiasch — view source

unread

Planning to add net_get_interfaces()
https://github.com/php/php-src/pull/2935/files for enumerating the
adapters on a system and their configured addresses.
Based on a combination of my own implementation and finding out
krakjoe and ab@ had a stalled version already.

If anyone wants an RFC I'll write one up, but this is pretty small so
mostly just offering for a look before I push.

-Sara

Ref: https://bugs.php.net/bug.php?id=17400
Ref: https://github.com/php/php-src/compare/master...weltling:17400

Looks good in theory, but without a lot of thought, how likely is this
to break/work on "supported" operating systems? (Which ones are that,
actually? http://php.net/manual/en/install.unix.php lists the BSDs and
Solaris and HP/UX)

I know, it explicitly mentions Windows and Linux - also probably someone
tried it on OSX, and I don't think (Free|Open|Net)BSD will be a big
problem here, but as I'm no expert on that - does that matter? Will it
need "just a little work" or could the more exotic ones be more problematic?

Greetings,
Florian

6 years ago by Sara Golemon — view source

unread

Looks good in theory, but without a lot of thought, how likely is this
to break/work on "supported" operating systems? (Which ones are that,
actually? http://php.net/manual/en/install.unix.php lists the BSDs and
Solaris and HP/UX)

I know, it explicitly mentions Windows and Linux - also probably someone
tried it on OSX, and I don't think (Free|Open|Net)BSD will be a big
problem here, but as I'm no expert on that - does that matter? Will it
need "just a little work" or could the more exotic ones be more problematic?

Without a comprehensive CI matrix to run diffs like this against,
there's no way to be absolutely certain it'll work everywhere.

That said, the config.m4 changes specifically test for the new APIs
being used. If they fail to compile in a standalone environment, the
new function isn't enabled for compilation in the main build. So, at
worst, we may find some OSs (AIX, Solaris, etc...) simply don't gain
the new functionality, but it shouldn't {knock on wood} cause any
builds to break. If it does, we have an entire year for interested
parties to catch it.

-Sara

6 years ago by Florian Anderiasch — view source

unread

Looks good in theory, but without a lot of thought, how likely is this
to break/work on "supported" operating systems? (Which ones are that,
actually? http://php.net/manual/en/install.unix.php lists the BSDs and
Solaris and HP/UX)

I know, it explicitly mentions Windows and Linux - also probably someone
tried it on OSX, and I don't think (Free|Open|Net)BSD will be a big
problem here, but as I'm no expert on that - does that matter? Will it
need "just a little work" or could the more exotic ones be more problematic?

Without a comprehensive CI matrix to run diffs like this against,
there's no way to be absolutely certain it'll work everywhere.

That said, the config.m4 changes specifically test for the new APIs
being used. If they fail to compile in a standalone environment, the
new function isn't enabled for compilation in the main build. So, at
worst, we may find some OSs (AIX, Solaris, etc...) simply don't gain
the new functionality, but it shouldn't {knock on wood} cause any
builds to break. If it does, we have an entire year for interested
parties to catch it.

Yeah, I know, and I'd agree on the "absolutely certain" part - but as
this is not an extension, I have somewhat of an uneasy feeling to even
merge this without some at least rudimentary testing on "all" OSes. I am
confident it would probably work on most and can be fixed on the others

but I see that as a TODO during the PR phase.

Speaking from a user PoV - if this simply doesn't work on some OSes for
no good reason.. that's a pretty leaky abstraction. If it's in the PHP
core, I'd assume it will work. Also I wouldn't count on it getting done,
just because there's one year of time and "we have no CI for that" is
not a good excuse in my book.

Don't get me wrong, this sounds like a useful feature - but (and my wild
guessing could be inaccurate) I think it's low-level enough that the
differences might matter.

Greetings,
Florian

6 years ago by Dan Ackroyd — view source

unread

if this simply doesn't work on some OSes for
no good reason.. that's a pretty leaky abstraction.

Cool. Does that mean that we can drop support for 32bit builds if
leaky abstractions are an important thing?

Looks good in theory, but without a lot of thought, how likely is this
to break/work on "supported" operating systems?

It's likely to work on platforms that have the C functions
implemented. Which is the level of guarantee that PHP aims for; "If
the platform implements a feature, PHP will expose it, but won't
always go out of its way to work around missing features".

just because there's one year of time and "we have no CI for that" is
not a good excuse in my book.

If this is important to you, please step up and do something about it.

If it is only of just enough importance to you that you send an email
to this list, asking other people to do a shedload of work to satisfy
your vague feelings, then as Sara said, committing it now gives people
a whole year to find issues with it, and then we can take action
against known issues, rather than worrying about unknown unknowns.

cheers
Dan
Ack

6 years ago by Florian Anderiasch — view source

unread

if this simply doesn't work on some OSes for
no good reason.. that's a pretty leaky abstraction.

Cool. Does that mean that we can drop support for 32bit builds if
leaky abstractions are an important thing?

If someone commits something that only works on 64bit and wasn't tested
a single time on a 32 bit machine - yes.

Looks good in theory, but without a lot of thought, how likely is this
to break/work on "supported" operating systems?

It's likely to work on platforms that have the C functions
implemented. Which is the level of guarantee that PHP aims for; "If
the platform implements a feature, PHP will expose it, but won't
always go out of its way to work around missing features".

Yes, I wrote that it's likely. Maybe I just didn't make myself clear
enough that I see a slight difference between something that's
relatively nicely encapsulated in the language itself versus something
that not only depends on some C headers, but maybe the whole network
stack of the OS.

just because there's one year of time and "we have no CI for that" is
not a good excuse in my book.

If this is important to you, please step up and do something about it.

If it is only of just enough importance to you that you send an email
to this list, asking other people to do a shedload of work to satisfy
your vague feelings, then as Sara said, committing it now gives people
a whole year to find issues with it, and then we can take action
against known issues, rather than worrying about unknown unknowns.

I didn't ask anybody to to any work. I saw this as a call for comments,
and Sara kind of addressed my concerns where I said I think this
might break.

Also I'm really sorry that I disappointed you by not managing to finish
testing on OpenBSD (which I actually started just before seeing this
mail) and I already sent some results for FreeBSD yesterday, which
showed a tiny issue.

Insert friendly suggestion to not throw stones when lingering near
breakable structures.

Greetings,
Florian

6 years ago by Sara Golemon — view source

unread

Also I'm really sorry that I disappointed you by not managing to finish
testing on OpenBSD (which I actually started just before seeing this
mail) and I already sent some results for FreeBSD yesterday, which
showed a tiny issue.

Yes! I saw that! Thank you. It wasn't so much a BSD issue as the fact
you did a bare build, while my default build happens to include the
sockets extension (which provides the AF_INET constant I was using in
the unit test). Thanks to your testing, I was able to refactor the
test slightly to not require ext/sockets be enabled in order to work.
:D

-Sara

6 years ago by Florian Anderiasch — view source

unread

Also I'm really sorry that I disappointed you by not managing to finish
testing on OpenBSD (which I actually started just before seeing this
mail) and I already sent some results for FreeBSD yesterday, which
showed a tiny issue.

Yes! I saw that! Thank you. It wasn't so much a BSD issue as the fact
you did a bare build, while my default build happens to include the
sockets extension (which provides the AF_INET constant I was using in
the unit test). Thanks to your testing, I was able to refactor the
test slightly to not require ext/sockets be enabled in order to work.
:D

-Sara

No worries, nothing in that was directed at you, Sara.
(But not sure if having to enable sockets proves my point or not, I
guess it did provide at least some value).

Good news: master after the merge works on OpenBSD, once the build is
fixed.

Somehow it doesn't match the
+#if HAVE_NET_IF_H
in net.c

I had to unconditionally include it (for testing), otherwise
IFF_BROADCAST and IFF_POINTOPOINT are undefined. (they are in
sys/net/if.h on OpenBSD, but I only tested 6.2).

Can do a proper bug report if needed, but maybe I get around to test
NetBSD as well.

Greetings,
Florian

6 years ago by Sara Golemon — view source

unread

Somehow it doesn't match the
+#if HAVE_NET_IF_H
in net.c

I had to unconditionally include it (for testing), otherwise
IFF_BROADCAST and IFF_POINTOPOINT are undefined. (they are in
sys/net/if.h on OpenBSD, but I only tested 6.2).

Can you send or gist your config.log? (Optionally redacted to the
net/if.h check if you want to do that yourself)

Are you saying you simply removed the #if HAVE_NET_IF_H check? Or
that you did that AND changed the include directive to #include <sys/net/if.h> ?

Can do a proper bug report if needed, but maybe I get around to test
NetBSD as well.

Thanks!

6 years ago by Matteo Beccati — view source

unread

Hi everyone,

Can do a proper bug report if needed, but maybe I get around to test
NetBSD as well.

Thanks!

I've just tested a NetBSD 7.0 i386 machine and I'm getting a few errors:

ext/standard/net.c: In function ‘php_inet_ntop’:
ext/standard/net.c:82:41: error: ‘NI_MAXHOST’ undeclared (first use in
this function)
zend_string *ret = zend_string_alloc(NI_MAXHOST, 0);
^
ext/standard/net.c:82:41: note: each undeclared identifier is reported
only once for each function it appears in
ext/standard/net.c:83:4: warning: implicit declaration of function
‘getnameinfo’ [-Wimplicit-function-declaration]
if (getnameinfo(addr, addrlen, ZSTR_VAL(ret), NI_MAXHOST, NULL, 0,
NI_NUMERICHOST) == SUCCESS) {
^
ext/standard/net.c:83:71: error: ‘NI_NUMERICHOST’ undeclared (first use
in this function)
if (getnameinfo(addr, addrlen, ZSTR_VAL(ret), NI_MAXHOST, NULL, 0,
NI_NUMERICHOST) == SUCCESS) {
^
ext/standard/net.c: At top level:
ext/standard/net.c:98:13: warning: ‘iface_append_unicast’ defined but
not used [-Wunused-function]
static void iface_append_unicast(zval *unicast, zend_long flags,
^
*** Error code 1

I'll see if I can get them fixed in the next few days.

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

6 years ago by Sara Golemon — view source

unread

I've just tested a NetBSD 7.0 i386 machine and I'm getting a few errors:

ext/standard/net.c: At top level:
ext/standard/net.c:98:13: warning: ‘iface_append_unicast’ defined but
not used [-Wunused-function]
static void iface_append_unicast(zval *unicast, zend_long flags,
^
*** Error code 1

Interesting, that tells me that getifaddrs() isn't available on your
system. I've pushed a fix for the include and the unused method, but
I'll have to spin up a NetBSD VM to figure out a way to support the
functionality on that platform. Thanks for testing!

-Sara

6 years ago by Florian Anderiasch — view source

unread

Somehow it doesn't match the
+#if HAVE_NET_IF_H
in net.c

I had to unconditionally include it (for testing), otherwise
IFF_BROADCAST and IFF_POINTOPOINT are undefined. (they are in
sys/net/if.h on OpenBSD, but I only tested 6.2).

Can you send or gist your config.log? (Optionally redacted to the
net/if.h check if you want to do that yourself)

Are you saying you simply removed the #if HAVE_NET_IF_H check? Or
that you did that AND changed the include directive to #include <sys/net/if.h> ?

Can do a proper bug report if needed, but maybe I get around to test
NetBSD as well.

Thanks!

Filed as https://bugs.php.net/bug.php?id=75582 - let's move the
discussion over there, sorry for the noise.

Greetings,
Florian

6 years ago by Niklas Keller — view source

unread

Hey Sara,

I'm not sure about the naming, but I think such a function is a good idea.
Why a net_* prefix when it's in ext/standard? I'd suggest
get_network_interfaces() instead.

As previously mentioned on Twitter [1], it could be really useful for us in
amphp/socket [2].

[1] https://twitter.com/kelunik/status/934075249452306433
[2] https://github.com/amphp/socket/issues/35

Regards, Niklas

2017-11-23 23:46 GMT+01:00 Sara Golemon pollita@php.net:

Planning to add net_get_interfaces()
https://github.com/php/php-src/pull/2935/files for enumerating the
adapters on a system and their configured addresses.
Based on a combination of my own implementation and finding out
krakjoe and ab@ had a stalled version already.

If anyone wants an RFC I'll write one up, but this is pretty small so
mostly just offering for a look before I push.

-Sara

Ref: https://bugs.php.net/bug.php?id=17400
Ref: https://github.com/php/php-src/compare/master...weltling:17400