Gaëtan Rivet <[email protected]> writes: > (Adding back the mailing list + original CCes to the thread.) >
something weird happened, cause patchwork got it: https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/#2672584 not a big deal, I'll check my side. > On Mon, Apr 26, 2021, at 19:09, Paolo Valerio wrote: >> 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? >> > > To nest the loops, you can use the __COUNTER__ macro, like so: > > /* Generate a unique name with the __COUNTER__ macro to allow nesting loops. > */ > #define OVS_STR_(x,y) x##y > #define OVS_STR(x, y) OVS_STR_(x,y) > /* There is one such 'stringify' macro in cmap.h as well, maybe it could be > shared in a util.h or similar. */ > Ok, I'll share the macro in util.h. I would split the patch in this case. > #define FOR_EACH_PORT_IN_RANGE__(curr, min, max, INAME) \ > for (uint16_t INAME = N_PORT_ATTEMPTS(curr, min, max); \ > INAME > 0; INAME--, NEXT_PORT_IN_RANGE(curr, min, max)) > > #define FOR_EACH_PORT_IN_RANGE(curr, min, max) \ > FOR_EACH_PORT_IN_RANGE__(curr, min, max, OVS_STR(idx, __COUNTER__)) > >> >> + 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 >> > > INIT_N_PORT_ATTEMPTS implies that the initialization is done. > In the suggestion above I changed to N_PORT_ATTEMPTS, if the assignment > happens > outside the macro. yes, makes sense. Thanks, Paolo _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
