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.

Sign up for GitHub

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

Jump to bottom

net: ip: igmp: add IGMPv3 support #65293

Merged
merged 3 commits into from Nov 21, 2023
Merged

net: ip: igmp: add IGMPv3 support #65293

merged 3 commits into from Nov 21, 2023

Conversation

IVandeVeire
Copy link
Contributor

@IVandeVeire IVandeVeire commented Nov 16, 2023 •
edited

Added IGMPv3 support by implementing the basics of RFC 3376. I used the already existing structure of IGMPv2 as reference without changing the existing IGMP api. IGMPv3 can be enabled by enabling CONFIG_NET_IPV4_IGMPV3.

@IVandeVeire IVandeVeire changed the title Add IGMPv3 support net: ip: igmp: add IGMPv3 support Nov 16, 2023
@IVandeVeire IVandeVeire force-pushed the add_igmpv3 branch 3 times, most recently from 7843a62 to f852baf Compare November 16, 2023 13:11
jukkar
jukkar reviewed Nov 16, 2023
View reviewed changes
subsys/net/ip/igmp.c Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
rlubos
rlubos reviewed Nov 16, 2023
View reviewed changes
include/zephyr/net/igmp.h Outdated
* @return Return 0 if joining was done, <0 otherwise.
*/
#if defined(CONFIG_NET_IPV4_IGMPV3)
int net_ipv4_igmpv3_join(struct net_if *iface, const struct in_addr *addr, bool include,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need/want to have a separate API for IGMPv3? I'd rather see it in a way, that we have a single net_ipv4_igmp_join/leave() API, and with Kconfig you select whether it should send IGMPv2 or IGMPv3 queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a separate API because IGMPv3 has some additional arguments/functionality. How would you add the filter groups to a request if the API remains unchanged? For leaving a group, Kconfig should do the job indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not saying the API has to remain unchanged, I just put under discussion whether it makes sense to have a separate API for IGMPv3 only. Right now it's a bit counter-intuitive for me, that even after enabling IGMPv3 most (if not all) of the networking components would still send IGMPv2 reports.

Just as an idea, net_ipv4_igmp_join() could accept an additional (optional) struct igmp_param * argument, with content dependent on the enabled IGMP version, with some reasonable fallback behavior if not provided (no filters for IGMPv3 if that makes sense). Net components that do not care about this additional functionality (is it just filtering or more?) could just skip the parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IGMPv3 does only support filters. I try your suggestion and see how that turns out.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @rlubos, the public APIs should not need to mention v2/v3 etc. but be generic as the caller does not need to care what IGMP version we are using.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the join/leave more generic. Now, the IGMP version is selected by the config. I added an optional param struct like @rlubos suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add extra tests and needed documentation for the change of the public API once the changes are on point.

rlubos
rlubos reviewed Nov 16, 2023
View reviewed changes
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
jukkar
jukkar reviewed Nov 20, 2023
View reviewed changes
subsys/net/ip/net_private.h Outdated Show resolved Hide resolved
subsys/net/ip/utils.c Outdated Show resolved Hide resolved
include/zephyr/net/igmp.h Outdated
* @return Return 0 if joining was done, <0 otherwise.
*/
#if defined(CONFIG_NET_IPV4_IGMPV3)
int net_ipv4_igmpv3_join(struct net_if *iface, const struct in_addr *addr, bool include,
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @rlubos, the public APIs should not need to mention v2/v3 etc. but be generic as the caller does not need to care what IGMP version we are using.

@IVandeVeire IVandeVeire force-pushed the add_igmpv3 branch 4 times, most recently from d9b7053 to 5e0b19d Compare November 20, 2023 11:20
@IVandeVeire
Copy link
Contributor Author

I will add extra tests and needed documentation for the change of the public API once the changes are on point.

@IVandeVeire
net: ip: utils: changed input arguments of igmp_checksum to net_pkt
bf7d437 
Added igmpv3 checksum function to make it possible to calculate the
checksum of a complete igmpv3 pkt at once.

Signed-off-by: Ibe Van de Veire <ibe.vandeveire@basalte.be>
@IVandeVeire IVandeVeire force-pushed the add_igmpv3 branch 2 times, most recently from 46e3c44 to 4fc5710 Compare November 20, 2023 15:12
rlubos
rlubos reviewed Nov 20, 2023
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the feedback so far. Spotted a few more nits, but otherwise looks pretty good to me.

include/zephyr/net/igmp.h Outdated Show resolved Hide resolved
include/zephyr/net/igmp.h Outdated Show resolved Hide resolved
include/zephyr/net/net_if.h Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
@IVandeVeire IVandeVeire force-pushed the add_igmpv3 branch 3 times, most recently from 4fab65d to 2dd7704 Compare November 20, 2023 16:45
jukkar
jukkar requested changes Nov 20, 2023
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Looks good, I found couple of minor nits that we could change.

include/zephyr/net/net_if.h Outdated Show resolved Hide resolved
include/zephyr/net/igmp.h Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Nov 21, 2023
jukkar
jukkar reviewed Nov 21, 2023
View reviewed changes
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
subsys/net/ip/igmp.c Outdated Show resolved Hide resolved
@IVandeVeire
net: ip: igmp: add igmpv3 support
5cf5f6b 
Added igmpv3 support based on the already existing structure for igmpv2.
The already existing api is not modified to prevent breaking exisiting
applications.

Signed-off-by: Ibe Van de Veire <ibe.vandeveire@basalte.be>
@IVandeVeire
doc: migration guide: add note about IGMP migration
5a39f0a 
Added note about the migration steps needed to support the new IGMP api.
The api now expects an additional argument used for joining an IGMPv3
group.

Signed-off-by: Ibe Van de Veire <ibe.vandeveire@basalte.be>
jukkar
jukkar approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

@IVandeVeire Thanks for the support, much appreciated!

rlubos
rlubos approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

LGTM

@carlescufi carlescufi merged commit a43e516 into zephyrproject-rtos:main Nov 21, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukkar jukkar jukkar approved these changes

@rlubos rlubos rlubos approved these changes

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@ssharks ssharks Awaiting requested review from ssharks

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

@kartben kartben Awaiting requested review from kartben

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

Assignees

@rlubos rlubos

Labels
area: Networking Release Notes To be mentioned in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@IVandeVeire @jukkar @rlubos @carlescufi @zephyrbot