[go: nahoru, domu]

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.

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

Added NetEndian versions of toIp and toIpPort #253

Merged
merged 3 commits into from
May 15, 2023
Merged

Added NetEndian versions of toIp and toIpPort #253

merged 3 commits into from
May 15, 2023

Conversation

Mis1eader-dev
Copy link
Contributor

Adds pure byte versions of toIp and toIpPort, they are compact and use less memory, while also accounting for ipv4 and ipv6.

@an-tao
Copy link
Owner
an-tao commented May 14, 2023

@Mis1eader-dev thanks so much for this PR, would you please add some unit tests for this?

@Mis1eader-dev
Copy link
Contributor Author

It's done, can we merge it?

@an-tao an-tao merged commit 5a34b09 into an-tao:master May 15, 2023
6 checks passed
@Mis1eader-dev
Copy link
Contributor Author
Mis1eader-dev commented May 17, 2023

There is one potential optimization that could be done for ipv6.
Currently ipv6 addresses are encoded into a string in its full form, meaning "::1" will get encoded as 16 bytes with toIpNetEndian(), compared to toIp()'s 3 bytes. Meaning the net endian version of toIp() is at a disadvantage in encoding ipv6 addresses that have too many zero bytes.
One way to improve this is to add an additional byte to both ipv4 and ipv6 that indicates the ip version, and for ipv6 to avoid conflicts with ipv4 addresses that have only 4 bytes.

Here is why this additional byte is necessary:
The ipv6 address "::0101:0101" must be encoded as std::string({ 1, 1, 1, 1 }), this would conflict with the ipv4 address "1.1.1.1", that also gets encoded as std::string({ 1, 1, 1, 1 }).
These conflicts can be avoided by appending one additional byte that indicates the ip version, std::string({ 6, 1, 1, 1, 1 }) and std::string({ 4, 1, 1, 1, 1 }), this still is more compact than toIp for a non-printable version of that function, but is not adhering to any standard as far as I am aware.

In my opinion it is worth making this change, this function must only be used in non-printable contexts only, such as rate limiting, and user identification.

I will start working on this improvement and add it as a default parameter for both toIpNetEndian and toIpPortNetEndian.

Edit: Actually never mind, it is not worth adding more overhead to the generating function for these addresses, the likelihood of user ipv6 addresses having too many zeros is too little for this to be considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants