Skip to content

net/netif: add function to get interface by name from a buffer#16709

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/net/netif_get_by_name_buf
Aug 4, 2021
Merged

net/netif: add function to get interface by name from a buffer#16709
miri64 merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/net/netif_get_by_name_buf

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri commented Aug 4, 2021

Contribution description

This adds a function to netif that allows to search an interface by name, using a buffer and length (like the one provided by the URI parser), instead a null-terminated string. tests/gnrc_netif has been extended accordingly.

Testing procedure

tests/gnrc_netif should pass.

Issues/PRs references

Split from #16233

@leandrolanzieri leandrolanzieri added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Aug 4, 2021
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Aug 4, 2021
@leandrolanzieri leandrolanzieri mentioned this pull request Aug 4, 2021
Merged
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Aug 4, 2021 •
edited
Loading

I like the idea. In fact, I think we should use this new function instead of netif_get_by_name, since the upper layer can always do:

netif_get_by_name_buffer(str, strlen(str));

I would be in favour of changing the API and deprecate the current netif_get_by_name API (and replace it with the signature of this new function).
What do you think?

EDIT: I just noticed netif_get_by_name is just a wrapper now.

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Aug 4, 2021

I would be in favour of changing the API and deprecate the current netif_get_by_name API (and replace it with the signature of this new function).
What do you think?

EDIT: I just noticed netif_get_by_name is just a wrapper now.

I don't really have a strong opinion here, there is no overhead in keeping the current function the way it is implemented.

jia200x
jia200x approved these changes Aug 4, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

ACK. tests/gnrc_netif passes:

make all test && echo "OK"
...
~~ SNIP  1 - size:  20 byte, type: NETTYPE_NETIF (-1)
if_pid: 6  rssi: -32768  lqi: 0
flags: 0x0
src_l2addr: 3E:E6:B5:22:FD:0B
dst_l2addr: 3E:E6:B5:22:FD:0A
~~ PKT    -  2 snips, total size:  61 byte

OK

Regarding the API change of netif_get_by_name, this can be discussed later.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Aug 4, 2021

Lol, I just added the same thing to #16705 ^^

miri64
miri64 reviewed Aug 4, 2021
View reviewed changes
sys/net/netif/netif.c Outdated
Comment on lines +78 to +81
netif_t *netif_get_by_name(const char *name)
{
return netif_get_by_name_buffer(name, strlen(name));
}
Copy link
Copy Markdown
Member

@miri64 miri64 Aug 4, 2021

Choose a reason for hiding this comment

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

This can be static inline in the header now.

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Aug 4, 2021

Choose a reason for hiding this comment

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

Moved to the header

miri64
miri64 requested changes Aug 4, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

The old style function could be inline (Github does not let me make a change request without a comment and I forgot to make one before...)

miri64
miri64 requested changes Aug 4, 2021
View reviewed changes
miri64
miri64 requested changes Aug 4, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Please squash immediately with my last suggestion.

miri64
miri64 reviewed Aug 4, 2021
View reviewed changes
This was referenced Aug 4, 2021
Merged
Merged
@leandrolanzieri leandrolanzieri force-pushed the pr/net/netif_get_by_name_buf branch from c9c6c89 to 78c415f Compare August 4, 2021 15:11
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Aug 4, 2021

Squashed directly!

@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 4, 2021
@miri64 miri64 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 4, 2021
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Aug 4, 2021

Ci found some issues

@leandrolanzieri leandrolanzieri force-pushed the pr/net/netif_get_by_name_buf branch from 78c415f to d2d1c7e Compare August 4, 2021 16:11
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Aug 4, 2021

Ci found some issues

Fixed, #include <string.h> was missing in netif.h

@miri64 miri64 merged commit 1b0152b into RIOT-OS:master Aug 4, 2021
@leandrolanzieri leandrolanzieri deleted the pr/net/netif_get_by_name_buf branch August 5, 2021 11:10
@leandrolanzieri leandrolanzieri added this to the Release 2021.10 milestone Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@jia200x jia200x jia200x approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework 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 Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

3 participants

@leandrolanzieri @jia200x @miri64