netif: Initial import of a common network interface API (second try) by miri64 · Pull Request #8841 · RIOT-OS/RIOT

miri64 · 2018-03-27T19:55:16Z

Contribution description

This is a way simpler redo of #6413. There is no network interface initialization (yet), but I plan to provide one in a follow-up. This PR just focusses on names, options and a exemplary GNRC implementation.

Issues/PRs references

Supersedes #6413.

kaspar030 · 2018-03-29T20:07:59Z

sys/net/gnrc/netif/_netif.c

+    if (netif != NULL) {
+        res += fmt_str(name, "if");
+        res += fmt_u16_dec(&name[res], netif->pid);
+    }


does this need to be null-terminated?

kaspar030 · 2018-03-29T20:09:59Z

sys/net/gnrc/netif/_netif.c

+{
+    gnrc_netif_t *netif = NULL;
+
+    if ((strlen(name) > 2) && (name[0] == 'i') && (name[1] == 'f')) {


if (strncmp(name, "if", 2) == 0) {

I have the feeling this would result in bigger code size, but I agree that this is better readable.

kaspar030 · 2018-03-29T20:14:59Z

sys/net/gnrc/netif/_netif.c

+
+    if ((strlen(name) > 2) && (name[0] == 'i') && (name[1] == 'f')) {
+        while ((netif = gnrc_netif_iter(netif)) != NULL) {
+            char pidstr[2];


I think it would be more efficient to convert the pid part of name to uint16 before the loop and then compare within the loop:

uint16_t _pid = scn_u32_dec(&name[2], 2)); if (_pid) { while ((netif = gnrc_netif_iter(netif)) != NULL) { if (_pid == netif->pid) { return (netif_t) _pid); } }

kaspar030 · 2018-03-29T20:16:16Z

sys/include/net/netif.h

+/**
+ * @brief   Network interface enumeration type
+ */
+typedef intptr_t netif_t;


If this typedef would be in the stack-specific implementation part, the API would be way more flexible.

Agreed, I wanted to do this after thinking about it a bit anyway ;-)

kaspar030 · 2018-03-29T20:24:42Z

Maybe an interface like this makes more sense with specific functions instead of netopt get/set?

miri64 · 2018-04-01T12:33:01Z

Maybe an interface like this makes more sense with specific functions instead of netopt get/set?

Does it? This would explode the API to a lot of functions (as well as the adaption layer, remember: for most stacks this is mostly piped through to the netdev).

miri64 · 2018-04-01T13:02:39Z

Addressed comments.

cgundogan

I like the idea of having a common interface to interact with different network interfaces. I am questioning the benefit of the gnrc specific netif_get_name() function, though .. IMO interface names are meant for a better user experience. Just converting a 0 to an if0 isn't cutting it. I'd suggest something like wpan0, eth0, ... instead. I am not blocking this PR because of such trivialities, though.

Regarding @kaspar030's comment: I am leaning more towards @miri64's answer. Having a getter/setter for each option in the API will produce clutter.

So, I suggest we get this merged as soon as my minor doc related comments are addressed.

cgundogan · 2018-05-28T06:09:19Z

sys/include/net/netif.h

+ * @return next network interface.
+ * @return @ref NETIF_INVALID, if there is no interface after @p last
+ */
+netif_t netif_iter(netif_t last);


doxygen will certainly complain here, because last is undocumented.

cgundogan · 2018-05-28T06:12:48Z

sys/include/net/netif.h

+ *
+ * @param[in] netif A network interface.
+ * @param[out] name The name of the interface. Must not be `NULL`. Must at least
+ *                  hold @ref NETIF_NAMELENMAX bytes.


I'd like a note here, that name must be null-terminated afterwards. At least this is what the gnrc specific function is doing.

cgundogan · 2018-05-28T06:16:47Z

sys/include/net/netif.h

+ *
+ * @note    Supposed to be implemented by the networking module
+ *
+ * @param[in] name  The name of an interface. Must not be `NULL`.


Here again I would expect a note about null-termination. The GNRC specific function doesn't need this, e.g., but another network stack may depend on it.

kaspar030 · 2018-05-29T11:01:06Z

Regarding @kaspar030's comment: I am leaning more towards @miri64's answer.

Ok, I thought about it, me too. 😄

cgundogan · 2018-05-29T11:33:23Z

Ok, I thought about it, me too. 😄

Wanna dismiss your review then? (:

kaspar030

untested ACK.

miri64 · 2018-05-29T11:42:40Z

IMO interface names are meant for a better user experience. Just converting a 0 to an if0 isn't cutting it. I'd suggest something like wpan0, eth0, ... instead. I am not blocking this PR because of such trivialities, though.

Since this would require some extra logic, I could prefer to do this as a feature later-on if it is wanted.

miri64 · 2018-05-29T11:48:19Z

@cgundogan I addressed your and Travis' comments.

cgundogan · 2018-05-29T15:50:27Z

@miri64 okay, looks good. please squash

miri64 · 2018-05-29T17:12:23Z

Squashed

cgundogan

ACK

miri64 · 2018-05-29T17:49:41Z

Fixed murdock issues.

miri64 · 2018-05-29T18:46:58Z

Only failing tests were tests/ps_schedstatistics, tests/shell, and tests/struct_tm_utility, which aren't related to this PR and according to @cladmi and @bergzand false positives. So I'm letting the tests finish, but then restart without tests activated.

miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Mar 27, 2018

miri64 mentioned this pull request Mar 27, 2018

netif: Initial import of a common network interface API #6413

Closed

kaspar030 reviewed Mar 29, 2018

View reviewed changes

kaspar030 requested changes Mar 29, 2018

View reviewed changes

tcschmidt requested a review from cgundogan May 25, 2018 10:03

cgundogan approved these changes May 28, 2018

View reviewed changes

kaspar030 approved these changes May 29, 2018

View reviewed changes

miri64 force-pushed the netif/api/initial2 branch from adf7ab7 to 21427fe Compare May 29, 2018 17:12

cgundogan approved these changes May 29, 2018

View reviewed changes

cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels May 29, 2018

miri64 and others added 3 commits May 29, 2018 19:49

netif: Initial import of a common network interface API

06947b7

gnrc_netif: add GNRC specific implementation of netif.h

a082e10

tests: test netif.h operations for GNRC

f97db64

miri64 force-pushed the netif/api/initial2 branch from 21427fe to f97db64 Compare May 29, 2018 17:49

miri64 removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label May 29, 2018

miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 29, 2018

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 29, 2018

miri64 merged commit fce5c75 into RIOT-OS:master May 29, 2018

miri64 deleted the netif/api/initial2 branch May 29, 2018 20:10

cladmi mentioned this pull request May 30, 2018

WIP try to make shell tests reliable #9250

Closed

miri64 mentioned this pull request Jun 4, 2018

netif: use where appropriate #9287

Open

8 tasks

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

miri64 commented Mar 27, 2018

Contribution description

Issues/PRs references

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!

kaspar030 commented Mar 29, 2018

Uh oh!

miri64 commented Apr 1, 2018

Uh oh!

miri64 commented Apr 1, 2018

Uh oh!

cgundogan left a comment

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!

kaspar030 commented May 29, 2018

Uh oh!

cgundogan commented May 29, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented May 29, 2018

Uh oh!

miri64 commented May 29, 2018

Uh oh!

cgundogan commented May 29, 2018

Uh oh!

miri64 commented May 29, 2018

Uh oh!

cgundogan left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented May 29, 2018

Uh oh!

miri64 commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants