odhcpd icon indicating copy to clipboard operation
odhcpd copied to clipboard

router: Add PREF64 (RFC 8781) support

Open oskar456 opened this issue 3 years ago • 1 comments

Fixes #182

Signed-off-by: Ondřej Caletka [email protected]

oskar456 avatar Jul 10 '22 21:07 oskar456

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.

ties avatar Oct 14 '22 09:10 ties

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

aalmenar avatar Oct 25 '22 21:10 aalmenar

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

cvmiller avatar Dec 01 '22 17:12 cvmiller

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

gtxaspec avatar Mar 29 '23 03:03 gtxaspec

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

BKPepe avatar May 15 '23 08:05 BKPepe

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

BKPepe avatar Jun 13 '23 06:06 BKPepe

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

jonathanunderwood avatar Jun 19 '23 09:06 jonathanunderwood

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

Ansuel avatar Jun 19 '23 09:06 Ansuel

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

oskar456 avatar Jun 21 '23 10:06 oskar456

@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.

  • Why this is needed? https://github.com/openwrt/odhcpd/pull/186/commits/b724d579b69a6dcf97462c4aaa6e573a8ab88915 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.
  • https://github.com/openwrt/odhcpd/pull/186/commits/d39cb803aa11e9bbfba82166e1d688015024396d Why? Does it cause problem to have a default value?
  • https://github.com/openwrt/odhcpd/pull/186/commits/2ed2b0e737b2ce3c9776831d9bbf106efb934486 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 &
  • https://github.com/openwrt/odhcpd/pull/186/commits/330fa4aeb3d77c0423dc89912de3b7506146392c Merge it to the first commit and also I think you got confused by the nitpick previously.

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.

Ansuel avatar Jun 22 '23 19:06 Ansuel

@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.

oskar456 avatar Jun 22 '23 20:06 oskar456

@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.

Ansuel avatar Jun 23 '23 12:06 Ansuel