-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
AK: Introduce IPAddressCidr datatype #25006
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(note to other reviewers: this is part of SIP6TF, so I was consulted on quite a bit of this code and may be biased)
Very nice work, and extensive tests, I like it. I'm not happy with the name though, so some bikeshedding may be required.
AK/IpAddressCidr.h
Outdated
constexpr IPv6Address first_address_of_subnet() const | ||
{ | ||
u8 address[16]; | ||
u8 free_bits = MAX_LENGTH - length(); | ||
|
||
if (free_bits != 128) { | ||
u128 mask = NumericLimits<u128>::max() >> free_bits; | ||
u8 address_mask[16]; | ||
|
||
memcpy(address_mask, &mask, sizeof(address_mask)); | ||
|
||
auto const* original_address = ip_address().to_in6_addr_t(); | ||
for (int i = 0; i < 16; i++) { | ||
address[i] = original_address[i] & address_mask[i]; | ||
} | ||
} | ||
|
||
return address; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically the same algorithm we have for the IPv6 address subnet logic I introduced recently. Do you want to reuse the algorithm nico suggested there (since it's quite a bit more concise)? It should also be constexpr capable.
AK/IpAddressCidr.h
Outdated
constexpr IPv6Address last_address_of_subnet() const | ||
{ | ||
u8 inverse_address_mask[16]; | ||
u8 address[16] = { 0 }; | ||
u8 free_bits = MAX_LENGTH - length(); | ||
u128 inverse_mask = NumericLimits<u128>::max(); | ||
|
||
if (free_bits != 128) { | ||
u8 address_mask[16]; | ||
u128 mask = NumericLimits<u128>::max() >> free_bits; | ||
inverse_mask = inverse_mask << (128 - free_bits); | ||
|
||
memcpy(address_mask, &mask, sizeof(address_mask)); | ||
|
||
auto const* original_address = ip_address().to_in6_addr_t(); | ||
for (int i = 0; i < 16; i++) { | ||
address[i] = original_address[i] & address_mask[i]; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here, I think the other algorithm can be adopted to fill one bits instead of zero bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly Nits on my end, that would likely need support from other places in the code base to resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer CIDR
AK/IpAddressCidr.h
Outdated
auto address_string = TRY(m_address.to_string()); | ||
|
||
#ifdef KERNEL | ||
TRY(builder.try_append(address_string->view())); | ||
#else | ||
TRY(builder.try_append(address_string)); | ||
#endif | ||
|
||
TRY(builder.try_append('/')); | ||
TRY(builder.try_appendff("{}", m_length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would just be a
#ifdef KERNEL
return KSting::formatted("{}/{}", m_address, m_length());
#else
return Sting::try_formatted("{}/{}", m_address, m_length());
#endif
but we're likely missing a custom formatter, are we?
Note that fallibility in the Userland case is not enforced (anymore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just doing
#ifdef KERNEL
ErrorOr<NonnullOwnPtr<Kernel::KString>> to_string() const
{
return KString::formatted("{}/{}", m_address, m_length);
}
#else
ErrorOr<String> to_string() const
{
return String::formatted("{}/{}", m_address, m_length);
}
#endif
works is this case both for IPv4 and IPv6 in hosts and serenity tests. I'll adopt this for formatting.
u32 mask = netmask().to_u32(); | ||
u32 first = ip_address().to_u32() & mask; | ||
return IPv4Address(first | ~mask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
Isn't this essentially
ip_address().to_u32() | ((1<<m_length) - 1)
But might be mistaken bc of endianness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire IPv4 address is stored in reverse host endianness so this won't work like this:
Running test 'should_find_last_in_subnet'.
FAIL: TestIPv4AddressCidr.cpp:44: EXPECT_EQ(address.last_address_of_subnet(), IPv4Address(192, 0, 2, 255)) failed with lhs=255.255.255.1 and rhs=192.0.2.255
AK/IpAddressCidr.h
Outdated
if (free_bits != 128) { | ||
NetworkOrdered<u128> mask = NumericLimits<u128>::max() << free_bits; | ||
auto address_mask = bit_cast<Array<u8, 16>>(mask); | ||
|
||
auto const* original_address = ip_address().to_in6_addr_t(); | ||
for (int i = 0; i < 16; i++) { | ||
address[i] = original_address[i] & address_mask[i]; | ||
} | ||
} | ||
|
||
auto inverse_address_mask = bit_cast<Array<u8, 16>>(inverse_mask); | ||
|
||
for (int i = 0; i < 16; i++) { | ||
address[i] = address[i] | inverse_address_mask[i]; | ||
} | ||
|
||
return address; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above/V4 version
template<> | ||
struct Formatter<IPv6AddressCidr> : Formatter<StringView> { | ||
ErrorOr<void> format(FormatBuilder& builder, IPv6AddressCidr value) | ||
{ | ||
return Formatter<StringView>::format(builder, TRY(value.to_string())->view()); | ||
} | ||
}; | ||
#else | ||
template<> | ||
struct Formatter<IPv6AddressCidr> : Formatter<StringView> { | ||
ErrorOr<void> format(FormatBuilder& builder, IPv6AddressCidr value) | ||
{ | ||
return Formatter<StringView>::format(builder, TRY(value.to_string())); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the OG commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant with this, is that this could mirror the actual implementation of the CIDR to_string method,
maybe even replace it.
With some template shenanigans (Ie DerivedFrom<IpAddressCIDR>
) all formatters could use the same code
template<> | ||
struct Formatter<IPv4AddressCidr> : Formatter<StringView> { | ||
ErrorOr<void> format(FormatBuilder& builder, IPv4AddressCidr value) | ||
{ | ||
return Formatter<StringView>::format(builder, TRY(value.to_string())->view()); | ||
} | ||
}; | ||
#else | ||
template<> | ||
struct Formatter<IPv4AddressCidr> : Formatter<StringView> { | ||
ErrorOr<void> format(FormatBuilder& builder, IPv4AddressCidr value) | ||
{ | ||
return Formatter<StringView>::format(builder, TRY(value.to_string())); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See OG commit
Hello! One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the |
I will squash this again later when the code itself is fine |
This commit introduces a CIDR datatype which will become handy especially for IPv6 to not have to deal with netmasks.