Add addr pool in rule #550
Conversation
NikitaUnisikhin
force-pushed
the
add_addr_pool_in_rule
branch
from
a73e7a5
to
d071192
Compare
sources/address.c
Outdated
| int od_address_range_read_prefix(od_address_range_t *address_range, char *prefix) | ||
| { | ||
| char *end = NULL; | ||
| long len = strtoul(prefix, &end, 10); |
There was a problem hiding this comment.
Why do we need to write an unsigned long value to a long variable? I think we should either change the type of the variable to unsigned long or use the strtol() function to convert the value to long.
There was a problem hiding this comment.
len can be <= 0 (str 55)
There was a problem hiding this comment.
but why do you use strtoul (to unsigned long)?
There was a problem hiding this comment.
oh yeah, thanks for the comment, I'll fix it
sources/address.c
Outdated
| if (len > 32) | ||
| return -1; | ||
| struct sockaddr_in *addr = (struct sockaddr_in *)&address_range->mask; | ||
| long mask; |
There was a problem hiding this comment.
Why do we need to store the mask as a long variable, when it can be perceived as a 32-bit integer? It might be more convenient to save it as an int32 or a uint32 type, since we do so on line 44 and in the call to the function on line 47. In both cases, it would seem more suitable to use a uint32 variable to me.
There was a problem hiding this comment.
I think that it is not good idea to used signed type for mask. There're exist some UB moments with signed integer types and bitwise operations
There was a problem hiding this comment.
thanks, fixed
sources/address.c
Outdated
| return (int) ch1 - (int) ch2; | ||
| } | ||
| if (ch1 == 0) | ||
| break; |
There was a problem hiding this comment.
Why do we believe that s1 is shorter than s2? Why are we only examining ch1 and not ch2?
There was a problem hiding this comment.
It's expected that ch1==ch2 at this point...
but, idk, there's strcasecmp() https://pubs.opengroup.org/onlinepubs/9699919799/functions/strcasecmp.html
There was a problem hiding this comment.
this case is considered in the next iteration of the loop
sources/address.c
Outdated
| found = false; | ||
| for (gai = gai_result; gai; gai = gai->ai_next) | ||
| { | ||
| if (gai->ai_addr->sa_family == client_sa->ss_family) |
There was a problem hiding this comment.
I'm not sure, but doesn't the "od_address_equals" function do all the things that happen inside this "if"?
There was a problem hiding this comment.
thanks, fixed
sources/address.c
Outdated
| bool od_address_range_equals(od_address_range_t *first, od_address_range_t *second) | ||
| { | ||
| if (first->is_hostname == second->is_hostname) | ||
| return first->string_value == second->string_value; |
There was a problem hiding this comment.
thanks, fixed
sources/address.c
Outdated
| return (int) ch1 - (int) ch2; | ||
| } | ||
| if (ch1 == 0) | ||
| break; |
There was a problem hiding this comment.
It's expected that ch1==ch2 at this point...
but, idk, there's strcasecmp() https://pubs.opengroup.org/onlinepubs/9699919799/functions/strcasecmp.html
sources/address.c
Outdated
| return 1; | ||
| } | ||
|
|
||
| uint32 od_address_bswap32(uint32 x) |
There was a problem hiding this comment.
BTW we have od_hba_bswap32().
But I'd prefer to have od_bswap32() once and for all :)
There was a problem hiding this comment.
thanks, done
sources/router.c
Outdated
| @@ -348,14 +349,24 @@ od_router_status_t od_router_route(od_router_t *router, od_client_t *client) | |||
|
|
|||
| /* match latest version of route rule */ | |||
| od_rule_t *rule; | |||
|
|
|||
| struct sockaddr_storage sa; | |||
| if (!client->is_watchdog) { | |||
There was a problem hiding this comment.
Why do we exempt watchdog? Can we exampt all outgoing clients, not just watchdog?
There was a problem hiding this comment.
thanks, now we exempt all internal clients
| return address_range; | ||
| } | ||
|
|
||
| int od_address_range_copy(od_address_range_t *src, od_address_range_t *dst) |
There was a problem hiding this comment.
I don't use malloc
| @@ -178,6 +178,8 @@ void od_rules_rule_free(od_rule_t *rule) | |||
| free(rule->db_name); | |||
| if (rule->user_name) | |||
| free(rule->user_name); | |||
| if (rule->address_range.string_value) | |||
There was a problem hiding this comment.
maybe free address_range too?
There was a problem hiding this comment.
I don't use malloc
sources/address.c
Outdated
| long mask; | ||
| struct sockaddr_in *addr = | ||
| (struct sockaddr_in *)&address_range->mask; | ||
| uint32 mask; | ||
| if (len > 0) | ||
| mask = (0xffffffffUL << (32 - (int)len)) & 0xffffffffUL; |
There was a problem hiding this comment.
it seems that you now don't need "& 0xffffffffUL" here
There was a problem hiding this comment.
thanks, fixed
NikitaUnisikhin
force-pushed
the
add_addr_pool_in_rule
branch
from
5704171
to
bca8f46
Compare
-- Added address to the rule. Address supported IPv4/IPv6 with mask and hostname
-- Сhanged the logs taking into account the new parameter
-- Сreated a module for working with addresses (address.c) also used the module in hba
-- Added tests for new functionality