From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: tcp softlro refactor check
To: Jan Klemkow <j.klemkow@wemelug.de>
Cc: tech@openbsd.org
Date: Tue, 22 Apr 2025 14:58:10 +0200

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.

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?  Where did you take this from?

#include <stdio.h>

int
main(int argc, char *argv[])
{
        int f_int = 0x8000;
        short f_short = 0x8000;
        int test = 0x8000;

        printf("int   (flag & test) != test %d\n", (f_int & test) != test);
        printf("int   (flag & ~test)        %d\n", (f_int & ~test));
        printf("short (flag & test) != test %d\n", (f_short  & test) != test);
        printf("short (flag & ~test)        %d\n", (f_short & ~test));

        return 0;
}

int   (flag & test) != test 0
int   (flag & ~test)        0
short (flag & test) != test 0
short (flag & ~test)        -65536

bluhm