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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 13 Sep 2024 14:32:07 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> ---
>  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.

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.

> -		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))

-- 
Stefano