On 2022/09/13 10:57, Todd C. Miller wrote: > On Sat, 09 Jul 2022 12:53:17 +0100, Stuart Henderson wrote: > > > I'm trying to teach ftp(1) to do something like gui web browsers do > > and reduce the HTTP/HTTPS connection timeout from the default (75 seconds) > > if there are multiple addresses behind a hostname. > > > > There's an existing connection timeout mechanism that I thought it > > might make sense to reuse... > > > > -w seconds > > For URL format connections to HTTP/HTTPS servers, abort a slow > > connection after seconds. > > Here's a diff to implement timed_connect(). It replaces the alarm > used for slow connections with non-blocking connect(2) and ppoll(2). > I've also replaced the other connect(2) + connect_wait() calls with > timed_connect() so the -w option now works for more that just http.
Oh great, that works very nicely for this use case, thank you. Connecting via a proxy also still works as I'd expect. I'm basically OK with that diff though I'd like to test the installer (I have some mkb/mkr running now). Then we could either have fw_update call ftp with something like "-w 12", or automatically lower ftp(1)'s default as with the diff in https://marc.info/?l=openbsd-tech&m=165736755609602&w=2 Looking at fw_update.sh I see there's already a timer mechanism there which I'm rather confused about because I've definitely seen the installer sit for a long time at that point before (checking svn log for my config files, I changed my PF rules in July when I ran into this with 7.1 autoinstalls)... Even if that shell timer was working as expected, the timeout in ftp is more useful as it gives a chance to move past a single downed server. > - todd > > Index: usr.bin/ftp/extern.h > =================================================================== > RCS file: /cvs/src/usr.bin/ftp/extern.h,v > retrieving revision 1.52 > diff -u -p -u -r1.52 extern.h > --- usr.bin/ftp/extern.h 2 Feb 2021 12:58:42 -0000 1.52 > +++ usr.bin/ftp/extern.h 13 Sep 2022 16:43:30 -0000 > @@ -62,6 +62,7 @@ > */ > > #include <sys/types.h> > +#include <sys/socket.h> > > void abort_remote(FILE *); > void abortpt(int); > @@ -75,7 +76,6 @@ void cmdabort(int); > void cmdscanner(int); > int command(const char *, ...); > int confirm(const char *, const char *); > -int connect_wait(int); > FILE *dataconn(const char *); > int foregroundproc(void); > int fileindir(const char *, const char *); > @@ -109,6 +109,7 @@ void sethash(int, char **); > void setpeer(int, char **); > void setttywidth(int); > char *slurpstring(void); > +int timed_connect(int s, const struct sockaddr *, socklen_t, int); > > __dead void usage(void); > > Index: usr.bin/ftp/fetch.c > =================================================================== > RCS file: /cvs/src/usr.bin/ftp/fetch.c,v > retrieving revision 1.209 > diff -u -p -u -r1.209 fetch.c > --- usr.bin/ftp/fetch.c 8 Sep 2022 11:12:44 -0000 1.209 > +++ usr.bin/ftp/fetch.c 13 Sep 2022 16:40:00 -0000 > @@ -173,14 +173,6 @@ url_encode(const char *path) > return (epath); > } > > -/* ARGSUSED */ > -static void > -tooslow(int signo) > -{ > - dprintf(STDERR_FILENO, "%s: connect taking too long\n", __progname); > - _exit(2); > -} > - > /* > * Copy a local file (used by the OpenBSD installer). > * Returns -1 on failure, 0 on success > @@ -604,14 +596,8 @@ noslash: > } > #endif /* !SMALL */ > > - if (connect_timeout) { > - (void)signal(SIGALRM, tooslow); > - alarmtimer(connect_timeout); > - } > - > - for (error = connect(fd, res->ai_addr, res->ai_addrlen); > - error != 0 && errno == EINTR; error = connect_wait(fd)) > - continue; > + error = timed_connect(fd, res->ai_addr, res->ai_addrlen, > + connect_timeout); > if (error != 0) { > save_errno = errno; > close(fd); > @@ -700,11 +686,6 @@ noslash: > } > #endif > > - if (connect_timeout) { > - signal(SIGALRM, SIG_DFL); > - alarmtimer(0); > - } > - > /* > * Construct and send the request. Proxy requests don't want leading /. > */ > @@ -1242,7 +1223,6 @@ aborthttp(int signo) > { > const char errmsg[] = "\nfetch aborted.\n"; > > - alarmtimer(0); > write(fileno(ttyout), errmsg, sizeof(errmsg) - 1); > longjmp(httpabort, 1); > } > Index: usr.bin/ftp/ftp.1 > =================================================================== > RCS file: /cvs/src/usr.bin/ftp/ftp.1,v > retrieving revision 1.123 > diff -u -p -u -r1.123 ftp.1 > --- usr.bin/ftp/ftp.1 27 Mar 2022 20:09:12 -0000 1.123 > +++ usr.bin/ftp/ftp.1 13 Sep 2022 16:49:38 -0000 > @@ -320,9 +320,9 @@ Forces > to show all responses from the remote server, as well > as report on data transfer statistics. > .It Fl w Ar seconds > -For URL format connections to HTTP/HTTPS servers, abort a > -slow connection after > -.Ar seconds . > +Wait for > +.Ar seconds > +for the remote server to connect before giving up. > .El > .Pp > The host with which > Index: usr.bin/ftp/ftp.c > =================================================================== > RCS file: /cvs/src/usr.bin/ftp/ftp.c,v > retrieving revision 1.107 > diff -u -p -u -r1.107 ftp.c > --- usr.bin/ftp/ftp.c 18 Nov 2019 04:37:35 -0000 1.107 > +++ usr.bin/ftp/ftp.c 13 Sep 2022 16:43:03 -0000 > @@ -212,9 +212,8 @@ hookup(char *host, char *port) > } > } > #endif /* !SMALL */ > - for (error = connect(s, res->ai_addr, res->ai_addrlen); > - error != 0 && errno == EINTR; error = connect_wait(s)) > - continue; > + error = timed_connect(s, res->ai_addr, res->ai_addrlen, > + connect_timeout); > if (error != 0) { > /* this "if" clause is to prevent print warning twice */ > if (verbose && res->ai_next) { > @@ -1509,10 +1508,8 @@ reinit: > } else > goto bad; > > - for (error = connect(data, &data_addr.sa, data_addr.sa.sa_len); > - error != 0 && errno == EINTR; > - error = connect_wait(data)) > - continue; > + error = timed_connect(data, &data_addr.sa, data_addr.sa.sa_len, > + connect_timeout); > if (error != 0) { > if (activefallback) { > (void)close(data); > Index: usr.bin/ftp/util.c > =================================================================== > RCS file: /cvs/src/usr.bin/ftp/util.c,v > retrieving revision 1.95 > diff -u -p -u -r1.95 util.c > --- usr.bin/ftp/util.c 2 Feb 2021 12:58:42 -0000 1.95 > +++ usr.bin/ftp/util.c 13 Sep 2022 16:46:26 -0000 > @@ -1077,6 +1077,84 @@ controlediting(void) > #endif /* !SMALL */ > > /* > + * connect(2) with an optional timeout if secs > 0. > + */ > +int > +timed_connect(int s, const struct sockaddr *name, socklen_t namelen, int > secs) > +{ > + struct timespec now, target, timebuf, *timeout = NULL; > + int flags, nready, optval, ret = -1; > + socklen_t optlen; > + struct pollfd pfd; > + > + if (secs > 0) { > + timebuf.tv_sec = secs; > + timebuf.tv_nsec = 0; > + timeout = &timebuf; > + clock_gettime(CLOCK_MONOTONIC, &target); > + timespecadd(&target, timeout, &target); > + } > + > + flags = fcntl(s, F_GETFL, 0); > + if (flags == -1) { > + warn("fcntl(F_GETFL)"); > + return -1; > + } > + if (!(flags & O_NONBLOCK)) { > + if (fcntl(s, F_SETFL, flags | O_NONBLOCK) == -1) { > + warn("fcntl(F_SETFL)"); > + return -1; > + } > + } > + > + ret = connect(s, name, namelen); > + if (ret == 0 || errno != EINPROGRESS) > + goto done; > + > + for (;;) { > + pfd.fd = s; > + pfd.events = POLLOUT; > + nready = ppoll(&pfd, 1, timeout, NULL); > + switch (nready) { > + case -1: > + if (errno != EINTR && errno != EAGAIN) { > + warn("ppoll"); > + goto done; > + } > + if (timeout == NULL) > + continue; > + clock_gettime(CLOCK_MONOTONIC, &now); > + timespecsub(&now, &target, timeout); > + if (timeout->tv_sec >= 0) > + continue; > + /* FALLTHROUGH */ > + case 0: > + errno = ETIMEDOUT; > + goto done; > + default: > + optlen = sizeof(optval); > + ret = getsockopt(s, SOL_SOCKET, SO_ERROR, &optval, > + &optlen); > + if (ret == 0 && optval != 0) { > + ret = -1; > + errno = optval; > + } > + goto done; > + } > + } > + > +done: > + if (!(flags & O_NONBLOCK)) { > + if (fcntl(s, F_SETFL, flags) == -1) { > + warn("fcntl(F_SETFL)"); > + ret = -1; > + } > + } > + > + return ret; > +} > + > +/* > * Wait for an asynchronous connect(2) attempt to finish. > */ > int >
