Add in support for network interface flags. by clalancette · Pull Request #2037 · giampaolo/psutil

clalancette · 2021-12-20T22:50:25Z

Summary

OS: Linux, macOS
Bug fix: no
Type: add more information to net_if_addrs()

Description

Add in support for network interface flags.

That is, return the raw flag integer as reported by getifaddrs()
on POSIX-compatible systems. On Windows systems (and any other
ones that do not support flags), return None instead.

I was careful to add the flags field to the end of the snicaddr named tuple so that the API wouldn't be broken.

Full disclosure: I've tested this on Linux and on macOS, and it seems to work OK on both of those. I have not tested on Windows, since I don't have a Windows machine handy.

I've made a few choices here that we could change:

I've decided to add this additional functionality to net_if_addrs() as opposed to net_if_stats() or adding a new API. That makes the most sense to me, but I'm happy to change it if you feel otherwise.
The flags are just the raw integer that comes from the return value of getifaddrs() on POSIX. The meaning of each of the bits is standardized by POSIX, so it should be the same on all platforms that support this. That said, we could make this "nicer" and possibly more Pythonic by expanding those bitfields into human-readable flags.
I decided to return "None" when the flags aren't supported (like on Windows). That seemed like a good way to go here, but if we wanted to make sure it was always an integer we could return -1 or something on Windows.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

giampaolo · 2021-12-21T12:04:22Z

These flags are pretty useless since there are no exposed constants to compare them against (IFF_BROADCAST, IFF_LOOPBACK, etc). A solution to this could be turning them into strings, like:

flags="multicast,loopback,running"

...similarly to psutil.disk_partitions()'s opts field. According to the man page, the flags are:

       SIOCGIFFLAGS, SIOCSIFFLAGS
              Get or set the active flag word of the device.  
              ifr_flags contains a bit mask of the following values:

                                      Device flags
              IFF_UP            Interface is running.
              IFF_BROADCAST     Valid broadcast address set.
              IFF_DEBUG         Internal debugging flag.
              IFF_LOOPBACK      Interface is a loopback interface.
              IFF_POINTOPOINT   Interface is a point-to-point link.
              IFF_RUNNING       Resources allocated.
              IFF_NOARP         No arp protocol, L2 destination address not
                                set.
              IFF_PROMISC       Interface is in promiscuous mode.
              IFF_NOTRAILERS    Avoid use of trailers.
              IFF_ALLMULTI      Receive all multicast packets.

              IFF_MASTER        Master of a load balancing bundle.
              IFF_SLAVE         Slave of a load balancing bundle.
              IFF_MULTICAST     Supports multicast
              IFF_PORTSEL       Is able to select media type via ifmap.
              IFF_AUTOMEDIA     Auto media selection active.
              IFF_DYNAMIC       The addresses are lost when the interface
                                goes down.
              IFF_LOWER_UP      Driver signals L1 up (since Linux 2.6.17)
              IFF_DORMANT       Driver signals dormant (since Linux 2.6.17)
              IFF_ECHO          Echo sent packets (since Linux 2.6.25)

What makes me skeptical about this is that most of the flags above seem to refer to the network interface itself, and not the individual addresses. E.g. associating "IFF_PROMISC", "IFF_PORTSEL" or "IFF_AUTOMEDIA" to a network address wouldn't make much sense.

As such, perhaps the right place to do this is net_if_stats() and not net_if_addrs():
https://psutil.readthedocs.io/en/latest/#psutil.net_if_stats

clalancette · 2021-12-21T13:33:55Z

flags="multicast,loopback,running"

That's fine by me. I'll go ahead and implement it that way instead.

As such, perhaps the right place to do this is net_if_stats() and not net_if_addrs():

OK, that sounds good to me as well. I'll add a new extension API for getting the flags and add it to the snicstats API instead.

clalancette · 2021-12-21T14:25:49Z

And this has all been done in b75a234. Please take a look when you get a chance.

clalancette · 2022-01-04T16:50:43Z

I've now rebased this onto the latest, and resolved the conflicts in HISTORY.rst based on the release. This is ready for another review when you have time.

clalancette · 2022-01-18T17:30:45Z

Rebased onto the latest again. Please take a look.

clalancette · 2022-08-11T14:46:42Z

Friendly ping on this one; do you think we could get this in?

giampaolo · 2022-08-12T15:42:31Z

Hi Chris and sorry for being late but life got busier. =) I re-read your patch and I'm happy to include this functionality. I don't like hard-coding C constants in python though. I prefer if that is done in C. I remember we do something like this in here:

psutil/psutil/_psutil_bsd.c

Line 730 in df3fba5

// see sys/mount.h

I guess you can do something like that and remove "," at the beginning/end of the resulting string if needed.

That is, return the raw flag integer as reported by getifaddrs() on POSIX-compatible systems. On Windows systems (and any other ones that do not support flags), return None instead. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2022-08-16T14:25:18Z

I re-read your patch and I'm happy to include this functionality. I don't like hard-coding C constants in python though. I prefer if that is done in C.

Thanks for the feedback. I went through and rewrote most of it in C, but in a slightly different way (see 1a5842b). In particular, what I'm doing there is constructing a list of strings to return to the Python layer, which I can then easily combine using join. Please take a look and let me know what you think.

By the way, it turns out that it is much, much better to do this at the C layer. The IFF_ bit flags are not standardized, and so different platforms can report different values there (for instance, on Linux IFF_MULTICAST is 0x1000, while on macOS it is 0x8000). So this is absolutely the right move, thanks for noticing it.

giampaolo · 2022-08-16T14:38:20Z

docs/index.rst


  .. versionchanged:: 5.7.3 `isup` on UNIX also checks whether the NIC is running.

+  .. versionchanged:: 5.9.1 *flags* field was added.


- .. versionchanged:: 5.9.1 *flags* field was added. + .. versionchanged:: 5.9.1 *flags* field was added on POSIX.

Fixed in e05f926.

giampaolo · 2022-08-16T14:39:30Z

docs/index.rst

  - **speed**: the NIC speed expressed in mega bits (MB), if it can't be
    determined (e.g. 'localhost') it will be set to ``0``.
  - **mtu**: NIC's maximum transmission unit expressed in bytes.
+  - **flags**: a string of comma-separate flags on the interface (may be ``None``).


I think an empty string is better than None. This way anybody can do if "running" in flags without pre-emptively checking if flag is not None.

please also list all the possible strings values.

Changed to an empty string everywhere below. Also listed all of the possible string values in e05f926.

giampaolo · 2022-08-16T14:41:27Z

psutil/_psaix.py


        duplex = duplex_map.get(duplex, NIC_DUPLEX_UNKNOWN)
-        ret[name] = _common.snicstats(isup, duplex, speed, mtu)
+        ret[name] = _common.snicstats(isup, duplex, speed, mtu, None)


Since psutil_net_if_is_running function was already working on AIX / SunOS, I guess you can enable this code on those platforms as well.

I've enabled it for AIX, as it is basically the same code. That said, I don't have access to an AIX machine, so I can't verify it there.

See my comment below about SunOS.

giampaolo · 2022-08-16T14:42:25Z

psutil/_psbsd.py

+            flag_list = []
+            for flagname, bit in _psposix.POSIX_NET_FLAGS:
+                if flags & (1 << bit):
+                    flag_list.append(flagname)


Yep, good call. Removed in e05f926.

giampaolo · 2022-08-16T14:44:01Z

psutil/_psutil_posix.c

+
+    sock = socket(AF_INET, SOCK_DGRAM, 0);
+    if (sock == -1)
+        return NULL;


You should add PyErr_SetFromErrno(PyExc_OSError);, then goto error;

Fixed in e05f926.

giampaolo · 2022-08-16T14:50:05Z

psutil/_pswindows.py

        if hasattr(_common, 'NicDuplex'):
            duplex = _common.NicDuplex(duplex)
-        ret[name] = _common.snicstats(isup, duplex, speed, mtu)
+        ret[name] = _common.snicstats(isup, duplex, speed, mtu, None)


let's return "" instead of None

Fixed in e05f926.

giampaolo · 2022-08-16T15:15:44Z

psutil/_psbsd.py

+                if flags & (1 << bit):
+                    flag_list.append(flagname)
+
+            output_flags = ','.join(flag_list)


please rename this variable to just flags

So I kept this as output_flags currently, as we still need to use flags for the list of flags. I'm happy to change the name of both if you want, just let me know what you would prefer.

giampaolo · 2022-08-16T15:15:49Z

psutil/_pslinux.py

                debug(err)
        else:
-            ret[name] = _common.snicstats(isup, duplex_map[duplex], speed, mtu)
+            output_flags = ','.join(flags)


please rename this variable to just flags

Same comment here.

giampaolo · 2022-08-16T15:16:16Z

psutil/_psosx.py

            if hasattr(_common, 'NicDuplex'):
                duplex = _common.NicDuplex(duplex)
-            ret[name] = _common.snicstats(isup, duplex, speed, mtu)
+            output_flags = ','.join(flags)


please rename this variable to just flags

Same comment here.

giampaolo · 2022-08-16T18:06:28Z

psutil/_pssunos.py

        if hasattr(_common, 'NicDuplex'):
            duplex = _common.NicDuplex(duplex)
-        ret[name] = _common.snicstats(isup, duplex, speed, mtu)
+        ret[name] = _common.snicstats(isup, duplex, speed, mtu, None)


let's enable this on sunos as well (see my previous comment)

So SunOS is a completely different beast than Linux, BSD, macOS, or Windows. Looking at the code in

psutil/psutil/_psutil_sunos.c

Line 1484 in 4446e3b

psutil_net_if_stats(PyObject* self, PyObject* args) {

, it looks like it doesn't even use the same ioctl (it is using something called SIOCGLIFFLAGS), and then it verifies that data with another structure (kstat?). So I don't think I can easily enable SunOS, and I don't have access to a SunOS machine.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2022-08-18T19:27:11Z

@giampaolo Thanks for the very detailed review! I've made a number of changes based on your review, though I opted not to make some of the changes you requested (individual comments have details why). Please take another look!

giampaolo · 2022-08-23T08:06:22Z

docs/index.rst

  - **speed**: the NIC speed expressed in mega bits (MB), if it can't be
    determined (e.g. 'localhost') it will be set to ``0``.
  - **mtu**: NIC's maximum transmission unit expressed in bytes.
+  - **flags**: a string of comma-separated flags on the interface (may be the empty string).


"may be AN empty string"

Fixed in d83c959.

giampaolo · 2022-08-23T08:07:37Z

docs/index.rst


  .. versionchanged:: 5.7.3 `isup` on UNIX also checks whether the NIC is running.

+  .. versionchanged:: 5.9.1 *flags* field was added on POSIX.


Fixed in d83c959.

giampaolo · 2022-08-23T08:24:21Z

psutil/_psutil_posix.c

+    }
+
+    return true;
+}


I don't like doing the flag comparison / check in here (if flags & flag_to_check ...). That should happen in the main function. The utility function (this one) should only add the item to the dict.

Also, instead of return true or false I would just return 1 or 0_, which is consistent with the rest of the code base and avoids including <stdbool.h> (I have some memories of this header file causing problems on some platform).

That's fair. I've switched this helper function to return an int, and I'm now doing the check in the main function. All in d83c959.

giampaolo · 2022-08-23T08:28:16Z

psutil/_psutil_posix.c

+
+    sock = socket(AF_INET, SOCK_DGRAM, 0);
+    if (sock == -1) {
+        PyErr_SetFromErrno(PyExc_OSError);


Please use PyErr_SetFromOSErrnoWithSyscall("socket(SOCK_DGRAM)");.
Since in this function we can fail for 2 different reasons, this will tell where the error originated from (socket() instead of ioctl()).

(sorry, I know I told you to PyErr_SetFromErrno use this but I forgot we have PyErr_SetFromOSErrnoWithSyscall)

No problem, it happens. Fixed in d83c959.

giampaolo · 2022-08-23T08:29:00Z

psutil/_psutil_posix.c

+    PSUTIL_STRNCPY(ifr.ifr_name, nic_name, sizeof(ifr.ifr_name));
+    ret = ioctl(sock, SIOCGIFFLAGS, &ifr);
+    if (ret == -1) {
+        PyErr_SetFromErrno(PyExc_OSError);


Please use PyErr_SetFromOSErrnoWithSyscall("ioctl(SIOCGIFFLAGS)");.

Fixed in d83c959.

psutil/_psutil_posix.c

giampaolo · 2022-08-23T08:34:50Z

psutil/tests/test_system.py

        for name, stats in nics.items():
            self.assertIsInstance(name, str)
-            isup, duplex, speed, mtu = stats
+            isup, duplex, speed, mtu, flags = stats


please add:

self.assertIsInstance(flags, str)

Fixed in d83c959.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2022-09-06T14:14:38Z

Sorry for the delay here (I was on vacation). I've now gone through and fixed up all of the latest issues; please take another look!

giampaolo · 2022-09-06T14:48:41Z

docs/index.rst

+    ``pointopoint``, ``notrailers``, ``running``, ``noarp``, ``promisc``,
+    ``allmulti``, ``master``, ``slave``, ``multicast``, ``portsel``,
+    ``dynamic``, ``oactive``, ``simplex``, ``link0``, ``link1``, ``link2``,
+    and ``d2`` (some flags are only available on certain platforms).


Please add . Availability: POSIX. After merge I'll try to take a look on how feasible it is to do this on Windows.

I've added it in 3082526 . Let me know if that is where you want it.

giampaolo · 2022-09-06T14:50:25Z

docs/index.rst


  .. versionchanged:: 5.7.3 `isup` on UNIX also checks whether the NIC is running.

+  .. versionchanged:: 5.9.2 *flags* field was added on POSIX.


I released 5.9.2 yesterday. Please set this to 5.9.3.

Fixed in 3082526

giampaolo · 2022-09-06T14:50:51Z

psutil/_psutil_posix.c

 #include <Python.h>
 #include <errno.h>
 #include <stdlib.h>
+#include <stdbool.h>


I think you can remove this, right?

Good call, removed in 3082526

giampaolo · 2022-09-06T14:52:10Z

I added some final comments but overall it's good to go.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2022-09-06T16:21:26Z

I added some final comments but overall it's good to go.

Thanks! Besides your comments, I also made small changes in 3082526 that should hopefully make linting happier.

giampaolo · 2022-09-06T16:22:53Z

Perfect. I'm happy with this change. Thanks.

clalancette · 2022-09-06T16:23:32Z

Thank you so much! I appreciate all of the good feedback and the merge of the PR.

giampaolo requested changes Aug 16, 2022

View reviewed changes

Fixes from review.

e05f926

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

giampaolo requested changes Aug 23, 2022

View reviewed changes

Fixes from review.

d83c959

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

giampaolo reviewed Sep 6, 2022

View reviewed changes

More fixes from review.

3082526

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

giampaolo merged commit 70eecaf into giampaolo:master Sep 6, 2022

clalancette deleted the clalancette/add-net-flags branch September 6, 2022 16:23

giampaolo added a commit that referenced this pull request Sep 6, 2022

add ifconfig test case for NIC flags re. to #2037

5cf1807

ben9923 mentioned this pull request Sep 9, 2022

Build musllinux wheels #2126

Closed


		.. versionchanged:: 5.7.3 `isup` on UNIX also checks whether the NIC is running.

		.. versionchanged:: 5.9.1 flags field was added.


		.. versionchanged:: 5.7.3 `isup` on UNIX also checks whether the NIC is running.

		.. versionchanged:: 5.9.2 flags field was added on POSIX.

Uh oh!

Conversation

clalancette commented Dec 20, 2021

Summary

Description

Uh oh!

giampaolo commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clalancette commented Dec 21, 2021

Uh oh!

clalancette commented Dec 21, 2021

Uh oh!

clalancette commented Jan 4, 2022

Uh oh!

clalancette commented Jan 18, 2022

Uh oh!

clalancette commented Aug 11, 2022

Uh oh!

giampaolo commented Aug 12, 2022

Uh oh!

clalancette commented Aug 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

giampaolo commented Dec 21, 2021 •

edited

Loading

giampaolo Sep 6, 2022 •

edited

Loading