On 2025-09-09 10:51 +02, Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Aug 30, 2025 at 01:28:45PM +0000, David Hill wrote:
>> On 8/30/25 6:23 AM, Stefan Sperling wrote:
>> > Ok by me, though I spotted one issue noted below which should be
>> > fixed before commit:
>
>> > > +		switch (ss.ss_family) {
>> > > +		case AF_INET:
>> > > +			ptb->dummybuf_len = 1500-28;
>> > 
>> > Missing a "break;" here.
>> > 
>> 
>> Good catch!  We could get the interface MTU instead of hardcoding 1500, but
>> that would require ioctl, so I left it as is.
>
> It seems this hasn't been committed yet?
>
> Still OK by me. Meanwhile I have tested it, too. Works as expected for
> me.

Reads OK to me. For some reason the diff doesn't apply for and I can't
figure out why *shrug*.

stsp, maybe you can commit it if dhill times out. Previously he asked me
to commit stuff on his behalf.

Few minor comments inline.


>
>> Index: usr.bin/tcpbench/tcpbench.c
>> ===================================================================
>> RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v
>> diff -u -p -r1.73 tcpbench.c
>> --- usr.bin/tcpbench/tcpbench.c	30 Dec 2024 21:19:29 -0000	1.73
>> +++ usr.bin/tcpbench/tcpbench.c	30 Aug 2025 13:25:30 -0000
>> @@ -61,7 +61,6 @@
>>  #define DEFAULT_PORT "12345"
>>  #define DEFAULT_STATS_INTERVAL 1000 /* ms */
>>  #define DEFAULT_BUF (256 * 1024)
>> -#define DEFAULT_UDP_PKT (1500 - 28) /* TODO don't hardcode this */
>>  #define TCP_MODE !ptb->uflag
>>  #define UDP_MODE ptb->uflag
>>  #define MAX_FD 1024
>> @@ -117,6 +116,7 @@ static void	print_tcp_header(void);
>>  static void	list_kvars(void);
>>  static void	check_kvar(const char *);
>>  static char **	check_prepare_kvars(char *);
>> +static void	dummybuf_prepare(struct statctx *);
>>  static void	stats_prepare(struct statctx *);
>>  static void	summary_display(void);
>>  static void	tcp_stats_display(unsigned long long, long double, float,
>> @@ -352,8 +352,45 @@ check_prepare_kvars(char *list)
>>  }
>> 
>>  static void
>> +dummybuf_prepare(struct statctx *sc)
>> +{
>> +	/*
>> +	 * Rationale,
>> +	 * If TCP, use a big buffer with big reads/writes.
>> +	 * If UDP, use a big buffer in server and a buffer the size of a
>> +	 * ethernet packet.
>> +	 */
>> +	if (ptb->dummybuf_len == 0 && (ptb->sflag || TCP_MODE))
>> +		ptb->dummybuf_len = DEFAULT_BUF;
>> +		

trailing tab

>> +	if (ptb->dummybuf_len == 0) {
>> +		struct sockaddr_storage ss;
>> +		socklen_t len = sizeof(ss);
>> +
>> +		if (getsockname(sc->fd, (struct sockaddr *)&ss, &len) == -1)
>> +			err(1, "getsockname");
>> +
>> +		switch (ss.ss_family) {
>> +		case AF_INET:
>> +			ptb->dummybuf_len = 1500-28;

I'd prefer 1500 - 28

>> +			break;
>> +		case AF_INET6:
>> +		default:
>> +			ptb->dummybuf_len = 1500-48;

1500 - 48

I wanted to write a diff that uses 1500 - sizeof(ip) - sizeof(udphdr)
and 1500 - sizeof(ip6_hdr) - sizeof(udphdr) but the diff doesn't apply...

>> +			break;
>> +		}
>> +	}
>> +
>> +	if ((ptb->dummybuf = malloc(ptb->dummybuf_len)) == NULL)
>> +		err(1, "malloc");
>> +	arc4random_buf(ptb->dummybuf, ptb->dummybuf_len);
>> +}
>> +
>> +static void
>>  stats_prepare(struct statctx *sc)
>>  {
>> +	dummybuf_prepare(sc);
>> +
>>  	sc->buf = ptb->dummybuf;
>>  	sc->buflen = ptb->dummybuf_len;
>> 
>> @@ -1276,19 +1313,6 @@ main(int argc, char **argv)
>>  	if (pledge("stdio id dns inet unix", NULL) == -1)
>>  		err(1, "pledge");
>> 
>> -	/*
>> -	 * Rationale,
>> -	 * If TCP, use a big buffer with big reads/writes.
>> -	 * If UDP, use a big buffer in server and a buffer the size of a
>> -	 * ethernet packet.
>> -	 */
>> -	if (!ptb->dummybuf_len) {
>> -		if (ptb->sflag || TCP_MODE)
>> -			ptb->dummybuf_len = DEFAULT_BUF;
>> -		else
>> -			ptb->dummybuf_len = DEFAULT_UDP_PKT;
>> -	}
>> -
>>  	bzero(&hints, sizeof(hints));
>>  	hints.ai_family = family;
>>  	if (UDP_MODE) {
>> @@ -1375,9 +1399,6 @@ main(int argc, char **argv)
>> 
>>  	/* Init world */
>>  	TAILQ_INIT(&sc_queue);
>> -	if ((ptb->dummybuf = malloc(ptb->dummybuf_len)) == NULL)
>> -		err(1, "malloc");
>> -	arc4random_buf(ptb->dummybuf, ptb->dummybuf_len);
>> 
>>  	timerclear(&mainstats.t_first);
>>  	mainstats.floor_mbps = INFINITY;
>> 
>> 
>

-- 
In my defence, I have been left unsupervised.