Skip to content

Add pref64 support (RFC8781)#179

Merged
robbat2 merged 10 commits intoradvd-project:masterfrom
zajdee:add_pref64_support
Jul 8, 2022
Merged

Add pref64 support (RFC8781)#179
robbat2 merged 10 commits intoradvd-project:masterfrom
zajdee:add_pref64_support

Conversation

@zajdee
Copy link
Copy Markdown
Contributor

@zajdee zajdee commented Jun 28, 2022

This pull requests adds support for the pref64 option, as defined in RFC8781. Its configuration is part of the interface config section and has only two parameters: the nat64prefix and the optional AdvValidLifetime, the prefix valid lifetime (which should not be smaller than the MaxRtrAdvInterval).

interface eth0
{
        nat64prefix 64:ff9b::/96
        {
                AdvValidLifetime 1800;
        };
};

@zajdee zajdee force-pushed the add_pref64_support branch from 6073673 to f842cf6 Compare June 30, 2022 11:44
@stappersg stappersg changed the title Add pref64 support (RFC8781) WIP: Add pref64 support (RFC8781) Jun 30, 2022
@stappersg
Copy link
Copy Markdown
Member

After seening additional commits, I marked this merge request as Work In Progress.

@zajdee
Copy link
Copy Markdown
Contributor Author

zajdee commented Jun 30, 2022

Hi @stappersg, so far the implementation seems to be working, I have removed one extra space and added a documentation entry.
What's the current process regarding workflow testing and review? I'd like to know if you want more to be added or if you have comments to the code added that I should work on.

Thank you.

Comment thread gram.y Outdated
Comment thread gram.y Outdated
Comment thread gram.y
Comment thread gram.y Outdated
Comment thread gram.y Outdated
Comment thread interface.c Outdated
Comment thread defaults.h Outdated
@robbat2
Copy link
Copy Markdown
Member

robbat2 commented Jul 5, 2022

  • reviewed code and enabled the workflow after that review (sorry, have to check because of other projects where PRs contained malicious payloads that used CPU in workflow).
  • Left comments on improvements, mostly style; functionality seems fine.

@zajdee
Copy link
Copy Markdown
Contributor Author

zajdee commented Jul 6, 2022

Hi @robbat2,
thank you for all the valuable inputs.

  • I have updated the code according to your code review (also had to amend the original commits as I have used an incorrect e-mail address there) and also simplified it (nat64prefix_init_defaults now sets the default value of AdvValidLifetime).
  • There also was an unknown option 38 reported when radvd was running in debug mode, therefore I have skipped option 38 checking in process.c.
  • Finally I have added option 38 parsing to radvdump.

Could you please re-run the workflows to ensure I didn't break things? Thanks.

@stappersg
Copy link
Copy Markdown
Member

Could you please re-run the workflows to ensure I didn't break things?

Re-run triggered.

@zajdee
Copy link
Copy Markdown
Contributor Author

zajdee commented Jul 7, 2022

Hi gents,
I have further cleaned up the code and have nothing more to add/remove right now.
If there will be further changes needed based on your code review or as a result of the workflow checks, I will incorporate those.
Otherwise it's ready to be merged.

Thank you for your time reviewing this pull request.

@stappersg
Copy link
Copy Markdown
Member

Re-run triggered

@stappersg stappersg changed the title WIP: Add pref64 support (RFC8781) Add pref64 support (RFC8781) Jul 8, 2022
@stappersg
Copy link
Copy Markdown
Member

I have further cleaned up the code and have nothing more to add/remove right now.

I have removed the Work In Progress marker from the title of this merge request.

Copy link
Copy Markdown
Member

@robbat2 robbat2 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution.

@robbat2 robbat2 merged commit a646066 into radvd-project:master Jul 8, 2022
@Neustradamus
Copy link
Copy Markdown
Member

2024-12-31: radvd 2.20 has been released with this nice PR, thanks to @zajdee :)
Never too late!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants