From: Jan Klemkow <jan@openbsd.org>
Subject: Re: tcp softlro refactor check
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 22 Apr 2025 20:29:55 +0200

On Tue, Apr 22, 2025 at 02:58:10PM GMT, Alexander Bluhm wrote:
> On Tue, Apr 22, 2025 at 01:42:36PM +0200, Jan Klemkow wrote:
> > On Tue, Apr 22, 2025 at 12:30:24AM GMT, Alexander Bluhm wrote:
> > > +
> > > +	/* Just ACK and PUSH TCP flags are allowed. */
> > > +	if (ISSET(ext->tcp->th_flags, TH_ACK|TH_PUSH) != ext->tcp->th_flags)
> > > +		return 0;
> > 
> > I know, the internal mechanism of ISSET macro allows this.  Please keep
> > it this way:
> > 
> > 	if (ISSET(ext->tcp->th_flags, ~(TH_ACK|TH_PUSH)))
> > 		return 0;
> > 
> > So, it is more readable.
> 
> No, it is not.  (flags & TEST) == TEST is a well established idiom.

In this case I would prefer it this way:

	if ((ext->tcp->th_flags & (TH_ACK|TH_PUSH)) != ext->tcp->th_flags)
		return 0;

> With ~ you have to think of the type and signedness of the variable.
> If flag is int and TEST is UL it will not work, although this combination
> is unlikely.
> What about signed short flag and integer TEST constant?  Will sign
> extension of bit 15 work with your idiom?

Sorry, your are right.  I missed the corner case of ~ here.

> Where did you take this from?

ISSET() reads like the question IS one of those bits SET?  And comparing
this answer to a value looks kind of odd to me.  Even that I know ISSET
is just ((a) & (b)).

I find just four case where we do something similar:

./net/route.c:	if (rt == NULL || ISSET(rt->rt_flags, flags) != flags) {

./kern/tty.c:	if ((error = (ISSET(c, TTY_ERRORMASK))) != 0) {
./kern/tty.c:	    (ISSET(c, TTY_CHARMASK) <= 037 && c != '\t' && c != '\n')) ||
./net/if_aggr.c:	if (ISSET(pi->lacp_state, bits) != ISSET(state, bits))
./net/route.c:	if (rt == NULL || ISSET(rt->rt_flags, flags) != flags) {

In all other cases (~600) we use '(flags & TEST) == flags' instead.


Anyway.

OK jan@

bye,
Jan