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

Subnet broadcasts #377

Merged
merged 1 commit into from
Jan 16, 2021
Merged

Subnet broadcasts #377

merged 1 commit into from
Jan 16, 2021

Conversation

MabezDev
Copy link
Contributor

I previously opened this before, but I think it got lost the repo ownership changes.

Currently smoltcp only responds to physical layer broadcasts (255.255.255.255), this PR enables the interface to respond to subnet broadcasts, as discussed in https://tools.ietf.org/html/rfc917 chapter 8.

Questions

  • Should this check be applied to all ip_addrs associated with the iface?

@MabezDev
Copy link
Contributor Author

Seems like the travis errors are unrelated, as they fail on current master too?

@whitequark
Copy link
Contributor

No idea what happened with tests, shouldn't that be impossible with protected branches, anyway?

@Dirbaio
Copy link
Member

Dirbaio commented Sep 21, 2020 •
edited
Loading

Yes, master is broken, with both stable and nightly. EDIT see #378 What's weird is that commits that Travis previously said passed tests are now not passing.

@MabezDev MabezDev closed this Dec 13, 2020
@MabezDev MabezDev reopened this Dec 13, 2020
@whitequark
Copy link
Contributor

whitequark commented Dec 25, 2020 •
edited
Loading

Should probably be squashed, looks reasonable otherwise.

Dirbaio
Dirbaio requested changes Dec 25, 2020
View reviewed changes
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

Should this check be applied to all ip_addrs associated with the iface?

I'd say yes. We process unicast packets for all our IPs, IMO it's quite strange that we only process broadcast packets for the first one.

Looks great otherwise! :)

Dirbaio
Dirbaio approved these changes Jan 16, 2021
View reviewed changes
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

Looks great now that it checks all our addrs!

Commits got somehow duplicated when merging, can you rebase on top of master?

@MabezDev
Copy link
Contributor Author

Looks great now that it checks all our addrs!

Commits got somehow duplicated when merging, can you rebase on top of master?

Will do, some tests are still failing so I'll fix those first and then squash.

@MabezDev MabezDev force-pushed the subnet-local-broadcasts branch 2 times, most recently from a27006d to 5b2cee5 Compare January 16, 2021 01:25
@MabezDev
Copy link
Contributor Author

Should be good to go :)

@Dirbaio
Copy link
Member

Dirbaio commented Jan 16, 2021

There's still a clippy failure :(

@MabezDev
Copy link
Contributor Author

Whoops, must have missed that! Should be fixed now, sorry about that :)

@MabezDev
subnet_broadcasts
c492ed2 
Adds `is_subnet_broadcast` to the ethernet interface which checks for
subnet broadcasts, which are discussed on page 8 in
https://tools.ietf.org/html/rfc917. The subnet broadcast addresses are
derived from the interfaces ipv4 addresses.
@MabezDev
Copy link
Contributor Author

Fairly minor change, but I realised only ipv4 has the concept of broadcasts, so I removed a pointless match, and instead just pass the ip directly.

@Dirbaio Dirbaio merged commit 9d8dbbf into smoltcp-rs:master Jan 16, 2021
@Dirbaio
Copy link
Member

Dirbaio commented Jan 16, 2021

Awesome! Thank you!

@Dirbaio Dirbaio mentioned this pull request Apr 7, 2021
Merged
@Dirbaio Dirbaio mentioned this pull request Jun 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dirbaio Dirbaio Dirbaio approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MabezDev @whitequark @Dirbaio