Re: bin/60443: ftp ASCII transfers are too slow

On 26-07-12 08:00, [email protected] via gnats wrote:
  | FTP ASCII transfers seem to interact badly with Nagle's algorithm
  | because ASCII xfers rely on libc's BUFSIZ (1KB) internal buffer.
  | 
  | If you write 1KB at a time (relatively slowly because you're doing
  | getc(3)/putc(3) to locale '\n'), then it looks the 200ms Nagle timeout
  | comes into play.
  | 
  | At that's what it seems to me from looking at the tcpdump packet times.


  | 
  | ---START patch ftp(1)---
  | diff -urN usr.bin/ftp.orig/ftp.c usr.bin/ftp/ftp.c
  | --- usr.bin/ftp.orig/ftp.c	2026-02-07 03:55:18.973125260 +0000
  | +++ usr.bin/ftp/ftp.c	2026-02-08 15:51:14.124404272 +0000
  | @@ -1728,6 +1728,23 @@
  |  	return (1);
  |  }
  |  
  | +static FILE *
  | +ftp_fdopen(int fd, const char *lmode)
  | +{
  | +	enum { BSIZ = XFERBUFMIN * 4 };	/* 4K (any >MTU) */


I think we could use a similar buffer sizing logic to what I
implemented in ftp.c 1.181 on 2026/02/07 in recvrequest():

	if (rcvbuf_size == 0)
		bufsize = XFERBUFMAX;
	else
		bufsize = MAX(XFERBUFMIN, MIN(rcvbuf_size, XFERBUFMAX));

(maybe this logic could be centralized for easier reuse).


Unless this will cause issues on low memory systems with large
stdio buffer and separate binary mode read buffer?


  | +	FILE *fp;
  | +	void* buf;
  | +
  | +	if ((fp = fdopen(fd, lmode)) == NULL)
  | +		return NULL;
  | +	if ((buf = ftp_malloc(BSIZ)) == NULL) {
  | +		fclose(fp);
  | +		return NULL;	
  | +	}
  | +	(void)setvbuf(fp, buf, _IOFBF, BSIZ);

Doesn't this leak "buf" because we've allocated it?

What if we just call setvbuf() with buf==NULL which
should let the stdio cleanup when the FILE is closed?

I've done a bit of reading into different setvbuf()
man pages (POSIX, Linux, NetBSD, macOS, cppreference), and it
seems that all support auto-allocating if buf==NULL and freeing
on close.

POSIX setvbuf(3) states (https://pubs.opengroup.org/onlinepubs/9699919799/functions/setvbuf.html):
	If buf is not a null pointer, the array it points to may be used
	instead of a buffer allocated by setvbuf() and the argument size
	specifies the size of the array; otherwise, size may determine
	the size of a buffer allocated by the setvbuf() function.

Linux setvbuf(3) states:
	If the argument buf is NULL, only the mode is affected;
	a new buffer will be allocated on the next read or write
	operation.

However, NetBSD setvbuf(3) states:
	(If the size argument is not zero but buf is NULL,
	a buffer of the given size will be allocated immediately,
	and released on close.  This is an extension to ANSI C;
	portable code should use a size of 0 with any NULL buffer.)

macOS has a variation of this - understandable since both are 4.xBSD
derived man pages.

cppreference.com (https://en.cppreference.com/c/io/setvbuf)
	If buffer is a null pointer, resizes the internal buffer to
	size.


regards,
Luke.