search

openwrt / odhcpd

This repository is a mirror of https://git.openwrt.org/?p=project/odhcpd.git. Pull requests will be accepted which will be merged in odhcpd.git
GNU General Public License v2.0
160 stars 96 forks source link

router: Add PREF64 (RFC 8781) support #186

Closed oskar456 closed 1 year ago

oskar456 commented 1 year ago

Fixes #182

Signed-off-by: Ondřej Caletka ondrej@caletka.cz

ties commented 1 year ago

Please merge this patch. It will make experimenting with using IPv6 only networks easier for users.

I wanted to experiment with this option after reading https://labs.ripe.net/author/ondrej_caletka_1/deploying-ipv6-mostly-access-networks/ and this option is needed to trigger the CLAT in MacOS.

aalmenar commented 1 year ago

Been testing this patch on a custom compiled odhcp package for my openwrt router, and its working beautifully. Really good work @oskar456

cvmiller commented 1 year ago

Thanks for the backend support. Will there be an update to LuCI as well for PREF64?

gtxaspec commented 1 year ago

tested, works. waiting for this to be merged =D

BKPepe commented 1 year ago

@Ansuel, could you please look at it? Would be nice to have this in upcoming OpenWrt release. 🙏

BKPepe commented 1 year ago

Guys, @dedeckeh and @Ansuel, kind reminder, please.

jonathanunderwood commented 1 year ago

@Ansuel Any chance of getting this in ahead of 23.05 release? Would need to be in the next RC, ideally.

Ansuel commented 1 year ago

Also is there a chance to have this supported on luci?

oskar456 commented 1 year ago

Nit resolved, rebased to current master. I am going to have a look on LuCI support (though that is orthogonal to this repository)

Ansuel commented 1 year ago

@jonathanunderwood I hope we can get this feature for release! Will give some priority since it seems many are interested in this.

@oskar456 First thing. Thanks for the patch and the change! Fell free to ask any support and all to have this merged.

I gave this another look with cold minds and I have some question I hope you can answer.

Aside from these question, I would ask you to provide a commit description even if redundant to the first commit Referencing the issue fixed and why this is needed.

If you need help on how to rebase commit feel free to ask.

oskar456 commented 1 year ago

@oskar456 First thing. Thanks for the patch and the change! Fell free to ask any support and all to have this merged.

Thank you for looking at it. I am already using it for almost a year so I will be very happy if it would be part of the upstream.

I gave this another look with cold minds and I have some question I hope you can answer.

  • Why this is needed? b724d57 Isn't the original implementation ok? Wile the second version is more optimized and compact, I found the first one more readable. Also I would just merge it to the first commit since it's part of the new code anyway.

The old version was probably correct but the code was extremely inefficient. Even the new version is much more readable than the description of Scaled Lifetime Processing in the RFC.

  • d39cb80 Why? Does it cause problem to have a default value?

I feel like yes, because that means that the router does something unexpected. Not doing anything for wrong input seems to be a better behaviour.

  • 2ed2b0e Also something I would merge to the first commit. Also personal taste I would declare the 2 mask not defined and define the value for each case. I would also use 0x0 for each 0 value just for consistency. Also maybe since we are ntonl to everything, I would call that just to the end when doing the &

That is actually a good idea. I have rewritten it in such way.

  • 330fa4a Merge it to the first commit and also I think you got confused by the nitpick previously.

Now I see it, I added the empty line to the right place.

Aside from these question, I would ask you to provide a commit description even if redundant to the first commit Referencing the issue fixed and why this is needed.

If you need help on how to rebase commit feel free to ask.

Thank you for support. I have added a slightly extensive description. If there's anything more I can do, please let me know.

Ansuel commented 1 year ago

@oskar456 all good! I will check one other pr and then I will bump the odhcpd version in openwrt main

Again thanks a lot for helping me In improve these changes formal wise.