fib: use return constants for universal_address_compare()
#5222
Conversation
BytesGalore
added
Type: bug
| @@ -132,7 +132,8 @@ static int fib_find_entry(fib_table_t *table, uint8_t *dst, size_t dst_size, | |||
|
|
|||
| int ret_comp = universal_address_compare(table->data.entries[i].global, dst, &match_size); | |||
| /* If we found an exact match */ | |||
| if ((ret_comp == 0) || (is_all_zeros_addr && (match_size == 0) && (ret_comp == 2))) { | |||
| if ((ret_comp == UNIVERSAL_ADDRESS_EQUAL) | |||
| || (is_all_zeros_addr && (match_size == 0) && (ret_comp == UNIVERSAL_ADDRESS_IS_ALL_ZERO_ADDRESS))) { | |||
There was a problem hiding this comment.
I am wondering: is it even necessary to check for is_all_zeros_addr && (match_size == 0) if ret_comp == UNIVERSAL_ADDRESS_IS_ALL_ZERO_ADDRESS?
There was a problem hiding this comment.
yes it is :),
is_all_zeros_addr says that dst is all 0, e.g. a default route prefix.
(ret_comp == UNIVERSAL_ADDRESS_IS_ALL_ZERO_ADDRESS) says table->data.entries[i].global is all 0, e.g. also a default route entry.
In such case, where both are all 0 and match_size == 0 the compared entries are equal, i.e. an exact match.
There was a problem hiding this comment.
hmm, and why do we need to check the match_size if is_all_zeros_addr points to the default route and if ret_comp is UNIVERSAL_ADDRESS_IS_ALL_ZERO_ADDRESS?
is there a case where match_size would not be 0?
There was a problem hiding this comment.
hmm, and why do we need to check the
match_size
we don't :) its just a relic from the former approach
|
@cgundogan I added a companion commit removing the obsolete check, thx for pointing me |
| @@ -547,7 +548,7 @@ int fib_get_destination_set(fib_table_t *table, uint8_t *prefix, | |||
|
|
|||
| for (size_t i = 0; i < table->size; ++i) { | |||
| if ((table->data.entries[i].global != NULL) && | |||
| (universal_address_compare_prefix(table->data.entries[i].global, prefix, prefix_size<<3) >= 0)) { | |||
| (universal_address_compare_prefix(table->data.entries[i].global, prefix, prefix_size<<3) > -ENOENT)) { | |||
There was a problem hiding this comment.
why > -ENOENT instead of != -ENOENT ?
There was a problem hiding this comment.
I guess I just want to clearly state that in this case any positive return means a successful match.
But since we only have -ENOENT as error return value you're right and != would be sufficient.
There was a problem hiding this comment.
If we want to state that a positive return means match, >=0could also work? But the current solution seems a bit... un-concise. sorry for the nitpicking ^^
There was a problem hiding this comment.
ok, then I change it to >= UNIVERSAL_ADDRESS_EQUAL
BytesGalore
added
the
CI: needs squashing
|
done. |
|
Cool! Then I don't have any more complaints.. Could you sqash please? |
BytesGalore
force-pushed
the
fib_use_universal_address_return_values
branch
from
a0abe44
to
5ca7bf4
Compare
Lotterleben
added
CI: ready for build
|
Let's see what travis says :) (ACK if he's happy) |
BytesGalore
force-pushed
the
fib_use_universal_address_return_values
branch
from
5ca7bf4
to
8589e8f
Compare
|
nice, thx for the review :) |
change: Use the introduced return values in 38d5fc2 for
universal_address_compare()across the fib.fix: notifying routing protocols used the outdated former return value (was previously 1 for successful compare) to consider if the registered prefix of a routing protocol matches the given address.
If the protocol registered the unspecified address as prefix no notification took place since the return value is
UNIVERSAL_ADDRESS_IS_ALL_ZERO_ADDRESS(2) now.