Re: [PATCH v2 03/10] tcp: Simplify ifdef logic in tcp_update_seqack_wnd()

On Tue, Sep 17, 2024 at 11:54:34PM +0200, Stefano Brivio wrote:

On Fri, 13 Sep 2024 14:32:07 +1000 David Gibson wrote:

...

This function has a block conditional on !snd_wnd_cap shortly before an #ifdef HAS_SND_WND. But snd_wnd_cap implies HAS_SND_WND (if !HAS_SND_WND, snd_wnd_cap is statically false).

Therefore, simplify this down to a single conditional with an else branch. While we're there, fix some improperly indented closing braces.

Signed-off-by: David Gibson --- tcp.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/tcp.c b/tcp.c index cba3f3bd..6733e7e3 100644 --- a/tcp.c +++ b/tcp.c @@ -1061,27 +1061,25 @@ int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn, conn->wnd_to_tap = MIN(new_wnd_to_tap >> conn->ws_to_tap, USHRT_MAX); goto out; - } - - if (!tinfo) { - if (prev_wnd_to_tap > WINDOW_DEFAULT) { - goto out; -} - tinfo = &tinfo_new; - if (getsockopt(s, SOL_TCP, TCP_INFO, tinfo, &sl)) { - goto out; -} - } - -#ifdef HAS_SND_WND - if ((conn->flags & LOCAL) || tcp_rtt_dst_low(conn)) { - new_wnd_to_tap = tinfo->tcpi_snd_wnd; } else {

I thought cppcheck would report else-after-goto, but it doesn't, just else-after-return. In any case, we could simplify further by avoid that else clause (and one level of indentation) in the whole block below.

Good idea, done.

It would also look more natural to me: we deal with if (!snd_wnd_cap) as a special case and go to 'out' in that special case, then we resume with the regular path.

I guess this is better than the original anyway and it's not a strong preference, so I can also apply this up to 4/10 as it is (or fix up on merge). The rest of the patches up to 4/10 look good to me.

Well, I've made the change now, so I might as well repost :).

...

- tcp_get_sndbuf(conn); - new_wnd_to_tap = MIN((int)tinfo->tcpi_snd_wnd, - SNDBUF_GET(conn)); + if (!tinfo) { + if (prev_wnd_to_tap > WINDOW_DEFAULT) { + goto out; + } + tinfo = &tinfo_new; + if (getsockopt(s, SOL_TCP, TCP_INFO, tinfo, &sl)) { + goto out; + } + } + + if ((conn->flags & LOCAL) || tcp_rtt_dst_low(conn)) { + new_wnd_to_tap = tinfo->tcpi_snd_wnd; + } else { + tcp_get_sndbuf(conn); + new_wnd_to_tap = MIN((int)tinfo->tcpi_snd_wnd, + SNDBUF_GET(conn)); + } } -#endif

new_wnd_to_tap = MIN(new_wnd_to_tap, MAX_WINDOW); if (!(conn->events & ESTABLISHED))

-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson