Hello Gaetan, thanks for the feedback
Gaëtan Rivet <[email protected]> writes: > On Fri, Apr 23, 2021, at 00:28, Paolo Valerio wrote: [...] >> + >> + int i, j, s_attempts, d_attempts; > > Why not use uint16_t here? > {curr,min,max}_{d,s}port are uint16_t and {s,d}_attemps will be set to values > derived from them. > i and j will then be compared against {s,d}_attempts, so it seems safer to > keep them all aligned. > ACK > Additionally, it seems s,d_attempts are unnecessary. > They are only used to know the number of NEXT_PORT_IN_RANGE() that should be > attempted. > Their names are slightly misleading (if they are counts of attempts, > n_attempts would be clearer), > but also the index could be initialized to the number of attempts remaining, > and decrease during the loop. > As the indexes are not useful within the loop, it seems ok. > > Furthermore, if they are not useful, could the indexes be masked completely? > Would it be acceptable > to declare them within the for() loop? I know it's should generally be > avoided, but I've seen a few places > where in-line declaration were used. In that case I think it's justified if > it makes the macro safer to use and simpler > to read. > Right, the indexes are not useful within the loop, and masking them would make the macro simpler. OTOH, declaring them within the for() and nesting the loops would lead to a warning (-Wshadow). If I didn't miss anything, and if you are ok with it, I would change it, based on your suggestions, like the following: uint16_t i, j; FOR_EACH_PORT_IN_RANGE(i, curr_dport, min_dport, max_dport) { nat_conn->rev_key.src.port = htons(curr_dport); FOR_EACH_PORT_IN_RANGE(j, curr_sport, min_sport, max_sport) { [...] } } #define FOR_EACH_PORT_IN_RANGE(idx, curr, min, max) \ for (INIT_N_PORT_ATTEMPTS(idx, curr, min, max); \ idx > 0; idx--, NEXT_PORT_IN_RANGE(curr, min, max)) WDYT? >> + FOR_EACH_PORT_IN_RANGE(i, d_attempts, curr_dport, min_dport, >> max_dport) { >> + nat_conn->rev_key.src.port = htons(curr_dport); >> + FOR_EACH_PORT_IN_RANGE(j, s_attempts, curr_sport, min_sport, >> max_sport) { >> + nat_conn->rev_key.dst.port = htons(curr_sport); >> + if (!conn_lookup(ct, &nat_conn->rev_key, >> + time_msec(), NULL, NULL)) { >> + return true; >> } >> - first_port = min_port; >> - port = first_port; >> - all_ports_tried = false; >> } >> } >> - return false; >> + >> + /* Check if next IP is in range and respin. Otherwise, notify >> + * exhaustion to the caller. */ >> +next_addr: >> + if (next_addr_in_range_guarded(&curr_addr, &min_addr, >> + &max_addr, &guard_addr, >> + conn->key.dl_type == >> htons(ETH_TYPE_IP))) { >> + return false; >> + } >> + >> + goto another_round; >> } >> >> static enum ct_update_res >> diff --git a/lib/conntrack.h b/lib/conntrack.h >> index 9553b188a..c68a83ccd 100644 >> --- a/lib/conntrack.h >> +++ b/lib/conntrack.h >> @@ -77,6 +77,14 @@ enum nat_action_e { >> NAT_ACTION_DST_PORT = 1 << 3, >> }; >> >> +#define NAT_ACTION_SNAT_ALL (NAT_ACTION_SRC | NAT_ACTION_SRC_PORT) >> +#define NAT_ACTION_DNAT_ALL (NAT_ACTION_DST | NAT_ACTION_DST_PORT) >> + >> +enum { >> + MIN_NAT_EPHEMERAL_PORT = 1024, >> + MAX_NAT_EPHEMERAL_PORT = 65535 >> +}; >> + >> struct nat_action_info_t { >> union ct_addr min_addr; >> union ct_addr max_addr; >> @@ -85,6 +93,28 @@ struct nat_action_info_t { >> uint16_t nat_action; >> }; >> >> +#define IN_RANGE(curr, min, max) \ >> + (curr >= min && curr <= max) >> + >> +#define NEXT_PORT_IN_RANGE(curr, min, max) \ >> + curr = (!IN_RANGE(curr, min, max) || curr == max) ? min : curr + 1 >> + >> +/* if the current port is out of range increase the attempts by >> + * one so that in the worst case scenario the current out of >> + * range port plus all the in-range ports get tested. >> + * Note that curr can be an out of range port only in case of >> + * source port (SNAT with port range unspecified or DNAT), >> + * furthermore the source port in the packet has to be less than >> + * MIN_NAT_EPHEMERAL_PORT. */ >> +#define INIT_ATT(att, curr, min, max) \ > > INIT_ATT is rather unclear about what the macro does. > It is then used only once, it would seem worth it to be more explicit. > > Maybe INIT_N_PORT_ATTEMPTS? > ACK, it's a better name >> + att = (!IN_RANGE(curr, min, max)) ? (max - min) + 2 : (max - min) + 1 >> + >> +/* loose in-range check, the first curr port can be any port out of >> + * the range. */ >> +#define FOR_EACH_PORT_IN_RANGE(idx, att, curr, min, max) \ >> + for (idx = 0, INIT_ATT(att, curr, min, max); idx < att; idx++, \ >> + NEXT_PORT_IN_RANGE(curr, min, max)) >> + > > All those macros seem to be useful only for the conntrack module. > Maybe they could be moved to conntrack-private.h instead? Good point, I'll move all the additions to conntrack-private.h Paolo _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
