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

EDNSSubnetOpts could be simpler #13704

Open
Habbie opened this issue Jan 11, 2024 · 3 comments · May be fixed by #15040
Open

EDNSSubnetOpts could be simpler #13704

Habbie opened this issue Jan 11, 2024 · 3 comments · May be fixed by #15040
Assignees
@omoerbeek
Labels
auth dnsdist enhancement rec
Milestone
common-helpneeded

Comments

@Habbie
Copy link
Member

Habbie commented Jan 11, 2024

it currently looks like this:

struct EDNSSubnetOpts
{
	Netmask source;
	Netmask scope;
};

In a review for #13441, @rgacogne pointed out that the two networks should never be different; just their prefix lengths. The encoder consistently uses the network from source and never even looks at the network from scope. This suggests that scope could be uint8_t instead.
@Habbie Habbie added this to the common-helpneeded milestone Jan 11, 2024
@omoerbeek
Copy link
Member

omoerbeek commented Jan 11, 2024 •
edited
Loading

Given the struct layout rules, I think the actual sizeof(EDNSSubnetOpts) would not change if scope would be an uint8_t.

@Habbie
Copy link
Member Author

Habbie commented Jan 11, 2024

Ah, yes, that might be true. I'll retitle "smaller" to "simpler"

@Habbie Habbie changed the title EDNSSubnetOpts could be smaller EDNSSubnetOpts could be simpler Jan 11, 2024
@rgacogne
Copy link
Member

It would certainly be less confusing :-)

@omoerbeek omoerbeek self-assigned this Jan 14, 2025
@omoerbeek omoerbeek linked a pull request Jan 15, 2025 that will close this issue
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@omoerbeek omoerbeek

Labels
auth dnsdist enhancement rec
Projects
None yet
Milestone
common-helpneeded
Development

Successfully merging a pull request may close this issue.

EDNSSubnetOpts refactoring omoerbeek/pdns
3 participants
@Habbie @rgacogne @omoerbeek