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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3066 bytes --]

On Tue, Sep 17, 2024 at 11:54:34PM +0200, Stefano Brivio wrote:
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]