Changeset View
Standalone View
contrib/netcat/netcat.c
Show All 26 Lines | |||||
* | * | ||||
* $FreeBSD$ | * $FreeBSD$ | ||||
*/ | */ | ||||
/* | /* | ||||
* Re-written nc(1) for OpenBSD. Original implementation by | * Re-written nc(1) for OpenBSD. Original implementation by | ||||
* *Hobbit* <hobbit@avian.org>. | * *Hobbit* <hobbit@avian.org>. | ||||
*/ | */ | ||||
#include <errno.h> | |||||
#include <stdio.h> | |||||
#include <sys/arb.h> | |||||
#include <sys/limits.h> | #include <sys/limits.h> | ||||
cem: Picking some random lines; I think the header ordering was right before? style(9):
```
Kernel… | |||||
Done Inline ActionsI think I'd prefer the third one, seems least intrusive, even if somewhat ugly. trasz: I think I'd prefer the third one, seems least intrusive, even if somewhat ugly. | |||||
#include <sys/types.h> | #include <sys/types.h> | ||||
#include <sys/sbuf.h> | |||||
#include <sys/socket.h> | #include <sys/socket.h> | ||||
#include <sys/sysctl.h> | #include <sys/sysctl.h> | ||||
#include <sys/qmath.h> | |||||
#include <sys/stats.h> | |||||
Not Done Inline ActionsDoesn't this include need to be conditional on WITH_STATS? Without it, the file may not be installed? cem: Doesn't this include need to be conditional on WITH_STATS? Without it, the file may not be… | |||||
Done Inline ActionsThe header will be installed, it comes from sys/sys/, not from lib/libstats/. trasz: The header will be installed, it comes from sys/sys/, not from lib/libstats/.
| |||||
#include <sys/time.h> | #include <sys/time.h> | ||||
#include <sys/uio.h> | #include <sys/uio.h> | ||||
#include <sys/un.h> | #include <sys/un.h> | ||||
#include <netinet/in.h> | #include <netinet/in.h> | ||||
#ifdef IPSEC | #ifdef IPSEC | ||||
#include <netipsec/ipsec.h> | #include <netipsec/ipsec.h> | ||||
#endif | #endif | ||||
Show All 32 Lines | |||||
#define BUFSIZE 16384 | #define BUFSIZE 16384 | ||||
/* Command Line Options */ | /* Command Line Options */ | ||||
int dflag; /* detached, no stdin */ | int dflag; /* detached, no stdin */ | ||||
int Fflag; /* fdpass sock to stdout */ | int Fflag; /* fdpass sock to stdout */ | ||||
unsigned int iflag; /* Interval Flag */ | unsigned int iflag; /* Interval Flag */ | ||||
int kflag; /* More than one connect */ | int kflag; /* More than one connect */ | ||||
int lflag; /* Bind to local port */ | int lflag; /* Bind to local port */ | ||||
int FreeBSD_Mflag; /* Measure using stats(3) */ | |||||
int Nflag; /* shutdown() network socket */ | int Nflag; /* shutdown() network socket */ | ||||
int nflag; /* Don't do name look up */ | int nflag; /* Don't do name look up */ | ||||
int FreeBSD_Oflag; /* Do not use TCP options */ | int FreeBSD_Oflag; /* Do not use TCP options */ | ||||
char *Pflag; /* Proxy username */ | char *Pflag; /* Proxy username */ | ||||
char *pflag; /* Localport flag */ | char *pflag; /* Localport flag */ | ||||
int rflag; /* Random ports flag */ | int rflag; /* Random ports flag */ | ||||
char *sflag; /* Source Address */ | char *sflag; /* Source Address */ | ||||
int tflag; /* Telnet Emulation */ | int tflag; /* Telnet Emulation */ | ||||
Show All 22 Lines | |||||
int remote_connect(const char *, const char *, struct addrinfo); | int remote_connect(const char *, const char *, struct addrinfo); | ||||
int timeout_connect(int, const struct sockaddr *, socklen_t); | int timeout_connect(int, const struct sockaddr *, socklen_t); | ||||
int socks_connect(const char *, const char *, struct addrinfo, | int socks_connect(const char *, const char *, struct addrinfo, | ||||
const char *, const char *, struct addrinfo, int, const char *); | const char *, const char *, struct addrinfo, int, const char *); | ||||
int udptest(int); | int udptest(int); | ||||
int unix_bind(char *); | int unix_bind(char *); | ||||
int unix_connect(char *); | int unix_connect(char *); | ||||
int unix_listen(char *); | int unix_listen(char *); | ||||
void FreeBSD_stats_setup(int); | |||||
Done Inline ActionsFreeBSD_print_measurements()? (Is there a real active upstream for this program? Edit: I guess we regularly import updates from OpenBSD, ok.) cem: FreeBSD_print_measurements()?
(Is there a real active upstream for this program? Edit: I… | |||||
void FreeBSD_stats_print(int); | |||||
void set_common_sockopts(int, int); | void set_common_sockopts(int, int); | ||||
int map_tos(char *, int *); | int map_tos(char *, int *); | ||||
void report_connect(const struct sockaddr *, socklen_t); | void report_connect(const struct sockaddr *, socklen_t); | ||||
void usage(int); | void usage(int); | ||||
ssize_t drainbuf(int, unsigned char *, size_t *); | ssize_t drainbuf(int, unsigned char *, size_t *); | ||||
ssize_t fillbuf(int, unsigned char *, size_t *); | ssize_t fillbuf(int, unsigned char *, size_t *); | ||||
#ifdef IPSEC | #ifdef IPSEC | ||||
Show All 28 Lines | main(int argc, char *argv[]) | ||||
socksv = 5; | socksv = 5; | ||||
host = NULL; | host = NULL; | ||||
uport = NULL; | uport = NULL; | ||||
sv = NULL; | sv = NULL; | ||||
signal(SIGPIPE, SIG_IGN); | signal(SIGPIPE, SIG_IGN); | ||||
while ((ch = getopt_long(argc, argv, | while ((ch = getopt_long(argc, argv, | ||||
"46DdEe:FhI:i:klNnoO:P:p:rSs:tT:UuV:vw:X:x:z", | "46DdEe:FhI:i:klMNnoO:P:p:rSs:tT:UuV:vw:X:x:z", | ||||
longopts, NULL)) != -1) { | longopts, NULL)) != -1) { | ||||
switch (ch) { | switch (ch) { | ||||
case '4': | case '4': | ||||
family = AF_INET; | family = AF_INET; | ||||
break; | break; | ||||
case '6': | case '6': | ||||
family = AF_INET6; | family = AF_INET6; | ||||
break; | break; | ||||
Show All 40 Lines | case 'i': | ||||
errx(1, "interval %s: %s", errstr, optarg); | errx(1, "interval %s: %s", errstr, optarg); | ||||
break; | break; | ||||
case 'k': | case 'k': | ||||
kflag = 1; | kflag = 1; | ||||
break; | break; | ||||
case 'l': | case 'l': | ||||
lflag = 1; | lflag = 1; | ||||
break; | break; | ||||
case 'M': | |||||
#ifndef WITH_STATS | |||||
errx(1, "-M requires stats(3) support"); | |||||
#else | |||||
FreeBSD_Mflag = 1; | |||||
#endif | |||||
break; | |||||
case 'N': | case 'N': | ||||
Nflag = 1; | Nflag = 1; | ||||
break; | break; | ||||
case 'n': | case 'n': | ||||
nflag = 1; | nflag = 1; | ||||
break; | break; | ||||
case 'o': | case 'o': | ||||
fprintf(stderr, "option -o is deprecated.\n"); | fprintf(stderr, "option -o is deprecated.\n"); | ||||
▲ Show 20 Lines • Show All 211 Lines • ▼ Show 20 Lines | for (;;) { | ||||
&len); | &len); | ||||
if (connfd == -1) { | if (connfd == -1) { | ||||
/* For now, all errnos are fatal */ | /* For now, all errnos are fatal */ | ||||
err(1, "accept"); | err(1, "accept"); | ||||
} | } | ||||
if (vflag) | if (vflag) | ||||
report_connect((struct sockaddr *)&cliaddr, len); | report_connect((struct sockaddr *)&cliaddr, len); | ||||
if (FreeBSD_Mflag) | |||||
FreeBSD_stats_setup(connfd); | |||||
readwrite(connfd); | readwrite(connfd); | ||||
close(connfd); | close(connfd); | ||||
} | } | ||||
if (family != AF_UNIX) | if (family != AF_UNIX) | ||||
close(s); | close(s); | ||||
else if (uflag) { | else if (uflag) { | ||||
if (connect(s, NULL, 0) < 0) | if (connect(s, NULL, 0) < 0) | ||||
▲ Show 20 Lines • Show All 334 Lines • ▼ Show 20 Lines | readwrite(int net_fd) | ||||
struct pollfd pfd[4]; | struct pollfd pfd[4]; | ||||
int stdin_fd = STDIN_FILENO; | int stdin_fd = STDIN_FILENO; | ||||
int stdout_fd = STDOUT_FILENO; | int stdout_fd = STDOUT_FILENO; | ||||
unsigned char netinbuf[BUFSIZE]; | unsigned char netinbuf[BUFSIZE]; | ||||
size_t netinbufpos = 0; | size_t netinbufpos = 0; | ||||
unsigned char stdinbuf[BUFSIZE]; | unsigned char stdinbuf[BUFSIZE]; | ||||
size_t stdinbufpos = 0; | size_t stdinbufpos = 0; | ||||
int n, num_fds; | int n, num_fds; | ||||
int stats_printed = 0; | |||||
ssize_t ret; | ssize_t ret; | ||||
/* don't read from stdin if requested */ | /* don't read from stdin if requested */ | ||||
if (dflag) | if (dflag) | ||||
stdin_fd = -1; | stdin_fd = -1; | ||||
/* stdin */ | /* stdin */ | ||||
pfd[POLL_STDIN].fd = stdin_fd; | pfd[POLL_STDIN].fd = stdin_fd; | ||||
Show All 10 Lines | readwrite(int net_fd) | ||||
/* stdout */ | /* stdout */ | ||||
pfd[POLL_STDOUT].fd = stdout_fd; | pfd[POLL_STDOUT].fd = stdout_fd; | ||||
pfd[POLL_STDOUT].events = 0; | pfd[POLL_STDOUT].events = 0; | ||||
while (1) { | while (1) { | ||||
/* both inputs are gone, buffers are empty, we are done */ | /* both inputs are gone, buffers are empty, we are done */ | ||||
if (pfd[POLL_STDIN].fd == -1 && pfd[POLL_NETIN].fd == -1 | if (pfd[POLL_STDIN].fd == -1 && pfd[POLL_NETIN].fd == -1 | ||||
&& stdinbufpos == 0 && netinbufpos == 0) { | && stdinbufpos == 0 && netinbufpos == 0) { | ||||
if (FreeBSD_Mflag && !stats_printed) | |||||
FreeBSD_stats_print(net_fd); | |||||
close(net_fd); | close(net_fd); | ||||
return; | return; | ||||
} | } | ||||
/* both outputs are gone, we can't continue */ | /* both outputs are gone, we can't continue */ | ||||
if (pfd[POLL_NETOUT].fd == -1 && pfd[POLL_STDOUT].fd == -1) { | if (pfd[POLL_NETOUT].fd == -1 && pfd[POLL_STDOUT].fd == -1) { | ||||
if (FreeBSD_Mflag && !stats_printed) | |||||
FreeBSD_stats_print(net_fd); | |||||
close(net_fd); | close(net_fd); | ||||
return; | return; | ||||
} | } | ||||
/* listen and net in gone, queues empty, done */ | /* listen and net in gone, queues empty, done */ | ||||
if (lflag && pfd[POLL_NETIN].fd == -1 | if (lflag && pfd[POLL_NETIN].fd == -1 | ||||
&& stdinbufpos == 0 && netinbufpos == 0) { | && stdinbufpos == 0 && netinbufpos == 0) { | ||||
if (FreeBSD_Mflag && !stats_printed) | |||||
FreeBSD_stats_print(net_fd); | |||||
close(net_fd); | close(net_fd); | ||||
return; | return; | ||||
} | } | ||||
/* help says -i is for "wait between lines sent". We read and | /* help says -i is for "wait between lines sent". We read and | ||||
* write arbitrary amounts of data, and we don't want to start | * write arbitrary amounts of data, and we don't want to start | ||||
* scanning for newlines, so this is as good as it gets */ | * scanning for newlines, so this is as good as it gets */ | ||||
if (iflag) | if (iflag) | ||||
sleep(iflag); | sleep(iflag); | ||||
/* poll */ | /* poll */ | ||||
num_fds = poll(pfd, 4, timeout); | num_fds = poll(pfd, 4, timeout); | ||||
/* treat poll errors */ | /* treat poll errors */ | ||||
if (num_fds == -1) { | if (num_fds == -1) { | ||||
close(net_fd); | close(net_fd); | ||||
err(1, "polling error"); | err(1, "polling error"); | ||||
Done Inline ActionsThere's another close() case here, although I agree it doesn't make sense to print stats in this instance (unexpected poll error). It doesn't make sense to close() immediately before err() exit, either, but I guess we inherited it from OpenBSD. cem: There's another close() case here, although I agree it doesn't make sense to print stats in… | |||||
} | } | ||||
/* timeout happened */ | /* timeout happened */ | ||||
if (num_fds == 0) | if (num_fds == 0) { | ||||
if (FreeBSD_Mflag) | |||||
FreeBSD_stats_print(net_fd); | |||||
return; | return; | ||||
} | |||||
Done Inline ActionsShouldn't we close and print measurements in this case? cem: Shouldn't we close and print measurements in this case? | |||||
Done Inline ActionsHm, you're right. trasz: Hm, you're right.
| |||||
/* treat socket error conditions */ | /* treat socket error conditions */ | ||||
for (n = 0; n < 4; n++) { | for (n = 0; n < 4; n++) { | ||||
if (pfd[n].revents & (POLLERR|POLLNVAL)) { | if (pfd[n].revents & (POLLERR|POLLNVAL)) { | ||||
pfd[n].fd = -1; | pfd[n].fd = -1; | ||||
} | } | ||||
} | } | ||||
/* reading is possible after HUP */ | /* reading is possible after HUP */ | ||||
▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Lines | if (pfd[POLL_STDOUT].revents & POLLOUT && netinbufpos > 0) { | ||||
pfd[POLL_STDOUT].events = 0; | pfd[POLL_STDOUT].events = 0; | ||||
/* buffer no longer full - poll net in again */ | /* buffer no longer full - poll net in again */ | ||||
if (netinbufpos < BUFSIZE) | if (netinbufpos < BUFSIZE) | ||||
pfd[POLL_NETIN].events = POLLIN; | pfd[POLL_NETIN].events = POLLIN; | ||||
} | } | ||||
/* stdin gone and queue empty? */ | /* stdin gone and queue empty? */ | ||||
if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0) { | if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0) { | ||||
if (pfd[POLL_NETOUT].fd != -1 && Nflag) | if (pfd[POLL_NETOUT].fd != -1 && Nflag) { | ||||
if (FreeBSD_Mflag) { | |||||
FreeBSD_stats_print(net_fd); | |||||
stats_printed = 1; | |||||
} | |||||
shutdown(pfd[POLL_NETOUT].fd, SHUT_WR); | shutdown(pfd[POLL_NETOUT].fd, SHUT_WR); | ||||
cemUnsubmitted Not Done Inline ActionsWhat's this one for? shutdown isn't close and we could still be reading from the remote TCP socket for some time, I think. cem: What's this one for? shutdown isn't close and we could still be reading from the remote TCP… | |||||
traszAuthorUnsubmitted Done Inline ActionsIt seems you can't call getsockopt(3) on a connection that has been shut down; in this particular case it trips this check in sys/netinet/tcp_usrreq.c:tcp_ctloutput(): if (inp->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) { INP_WUNLOCK(inp); return (ECONNRESET); } trasz: It seems you can't call getsockopt(3) on a connection that has been shut down; in this… | |||||
cemUnsubmitted Not Done Inline ActionsHuh. That seems like a kernel bug to me, although obviously extremely tangential to this review. No objection to this for now. cem: Huh. That seems like a kernel bug to me, although obviously extremely tangential to this… | |||||
} | |||||
pfd[POLL_NETOUT].fd = -1; | pfd[POLL_NETOUT].fd = -1; | ||||
} | } | ||||
/* net in gone and queue empty? */ | /* net in gone and queue empty? */ | ||||
if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0) { | if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0) { | ||||
pfd[POLL_STDOUT].fd = -1; | pfd[POLL_STDOUT].fd = -1; | ||||
} | } | ||||
} | } | ||||
} | } | ||||
▲ Show 20 Lines • Show All 202 Lines • ▼ Show 20 Lines | if (write(s, "X", 1) == 1) | ||||
ret = 1; | ret = 1; | ||||
else | else | ||||
ret = -1; | ret = -1; | ||||
} | } | ||||
return (ret); | return (ret); | ||||
} | } | ||||
void | void | ||||
FreeBSD_stats_setup(int s) | |||||
{ | |||||
if (setsockopt(s, IPPROTO_TCP, TCP_STATS, | |||||
Done Inline ActionsMaybe this is just me, but sb is commonly used for struct sbuf pointers, so the name here makes reading the following code a little confusing to me. You don't have to make any change if you don't want to, but I'd probably use a distinct name for the statsblob, like stats or statsb. cem: Maybe this is just me, but `sb` is commonly used for `struct sbuf` pointers, so the name here… | |||||
&FreeBSD_Mflag, sizeof(FreeBSD_Mflag)) == -1) | |||||
err(1, "enable TCP_STATS gathering"); | |||||
} | |||||
void | |||||
Done Inline ActionsDon't cast malloc in C cem: Don't cast malloc in C | |||||
FreeBSD_stats_print(int s) | |||||
{ | |||||
#ifdef WITH_STATS | |||||
struct statsblob *statsb; | |||||
struct sbuf *sb; | |||||
socklen_t sockoptlen; | |||||
int error; | |||||
Done Inline ActionsThis kind of retry classically has the problem of never catching up to a growing structure. Although I guess if the socket is mostly shutdown maybe we can reasonably expect the true size to be static. Nevermind! :) cem: This kind of retry classically has the problem of never catching up to a growing structure. | |||||
sockoptlen = 2048; | |||||
statsb = malloc(sockoptlen); | |||||
Not Done Inline ActionsI think you intended to initialize error rather than errno; the former makes the logic more correct (it's not now) and the latter is already set by realloc() on failure. But I'd suggest just doing err(1, "realloc"); here instead; discussed below. cem: I think you intended to initialize `error` rather than `errno`; the former makes the logic more… | |||||
if (statsb == NULL) | |||||
err(1, "malloc"); | |||||
error = getsockopt(s, IPPROTO_TCP, TCP_STATS, statsb, &sockoptlen); | |||||
if (error != 0) { | |||||
if (errno == EOVERFLOW && statsb->cursz > sockoptlen) { | |||||
/* Retry with a larger size. */ | |||||
Not Done Inline ActionsThis is not a great err message; it probably makes sense to at least specify TCP and stats. And it's totally misleading if realloc failed. cem: This is not a great err message; it probably makes sense to at least specify TCP and stats. | |||||
Done Inline ActionsNot sure what's the best way to fix it. What do you think about this version? trasz: Not sure what's the best way to fix it. What do you think about this version?
| |||||
Not Done Inline ActionsLooks great to me, thanks cem: Looks great to me, thanks | |||||
sockoptlen = statsb->cursz; | |||||
statsb = realloc(statsb, sockoptlen); | |||||
if (statsb == NULL) | |||||
err(1, "realloc"); | |||||
error = getsockopt(s, IPPROTO_TCP, TCP_STATS, | |||||
statsb, &sockoptlen); | |||||
Done Inline ActionsThis function doesn't set errno on failure, it just returns a value. So you want errc(1, error, "stats_blob_tostr"); cem: This function doesn't set `errno` on failure, it just returns a value. So you want `errc(1… | |||||
} | |||||
if (error != 0) { | |||||
warnx("getsockopt(TCP_STATS) failed; " | |||||
allanjudeUnsubmitted Done Inline ActionsThis can give the wrong error message sometimes: nc: getsockopt(TCP_STATS) failed; kernel built without "options STATS"? We should not emit the "kernel built without STATS" in this common case, it is misleading allanjude: This can give the wrong error message sometimes:
nc: getsockopt(TCP_STATS) failed; kernel… | |||||
"kernel built without \"options STATS\"?"); | |||||
err(1, "getsockopt"); | |||||
} | |||||
} | |||||
sb = sbuf_new_auto(); | |||||
error = stats_blob_tostr(statsb, sb, SB_STRFMT_JSON, SB_TOSTR_META); | |||||
if (error != 0) | |||||
errc(1, error, "stats_blob_tostr"); | |||||
error = sbuf_finish(sb); | |||||
if (error != 0) | |||||
err(1, "sbuf_finish"); | |||||
fprintf(stderr, "%s\n", sbuf_data(sb)); | |||||
#endif | |||||
} | |||||
void | |||||
set_common_sockopts(int s, int af) | set_common_sockopts(int s, int af) | ||||
{ | { | ||||
int x = 1; | int x = 1; | ||||
if (Sflag) { | if (Sflag) { | ||||
if (setsockopt(s, IPPROTO_TCP, TCP_MD5SIG, | if (setsockopt(s, IPPROTO_TCP, TCP_MD5SIG, | ||||
&x, sizeof(x)) == -1) | &x, sizeof(x)) == -1) | ||||
err(1, NULL); | err(1, NULL); | ||||
Show All 23 Lines | if (setsockopt(s, SOL_SOCKET, SO_RCVBUF, | ||||
err(1, "set TCP receive buffer size"); | err(1, "set TCP receive buffer size"); | ||||
} | } | ||||
if (Oflag) { | if (Oflag) { | ||||
if (setsockopt(s, SOL_SOCKET, SO_SNDBUF, | if (setsockopt(s, SOL_SOCKET, SO_SNDBUF, | ||||
&Oflag, sizeof(Oflag)) == -1) | &Oflag, sizeof(Oflag)) == -1) | ||||
err(1, "set TCP send buffer size"); | err(1, "set TCP send buffer size"); | ||||
} | } | ||||
if (FreeBSD_Oflag) { | if (FreeBSD_Oflag) { | ||||
if (setsockopt(s, IPPROTO_TCP, TCP_NOOPT, | if (setsockopt(s, IPPROTO_TCP, TCP_NOOPT, | ||||
&FreeBSD_Oflag, sizeof(FreeBSD_Oflag)) == -1) | &FreeBSD_Oflag, sizeof(FreeBSD_Oflag)) == -1) | ||||
Not Done Inline ActionsIs the same proto:key really overloaded for both enabling statistics gathering and dumping the output blob? Can programs actually tell if TCP_STATS is enabled or does getsockopt(TCP, TCP_STATS) try to dump a stats blob into an int? cem: Is the same proto:key really overloaded for both enabling statistics gathering and dumping the… | |||||
Done Inline ActionsI'll investigate. trasz: I'll investigate.
| |||||
err(1, "disable TCP options"); | err(1, "disable TCP options"); | ||||
} | } | ||||
if (FreeBSD_Mflag) | |||||
FreeBSD_stats_setup(s); | |||||
#ifdef IPSEC | #ifdef IPSEC | ||||
if (ipsec_policy[0] != NULL) | if (ipsec_policy[0] != NULL) | ||||
add_ipsec_policy(s, af, ipsec_policy[0]); | add_ipsec_policy(s, af, ipsec_policy[0]); | ||||
if (ipsec_policy[1] != NULL) | if (ipsec_policy[1] != NULL) | ||||
add_ipsec_policy(s, af, ipsec_policy[1]); | add_ipsec_policy(s, af, ipsec_policy[1]); | ||||
#endif | #endif | ||||
} | } | ||||
▲ Show 20 Lines • Show All 162 Lines • Show Last 20 Lines |
Picking some random lines; I think the header ordering was right before? style(9):
I'm guessing that you made this change because sys/types.h cowardly refuses to define the same types in userspace as it does in the kernel, and thus you need at least stdbool and probably stdint, if not others, before the sys/qmath or sys/stats or sys/arb includes?
I might fix that by just having those sys/ headers recursively include their dependencies in userspace builds only :-/. Or probably more contentiously, sys/types.h could be modified to include the userspace headers for the types it normally defines for the kernel.
You could also just live with a mostly style-correct and unchanged ordering, but add the handful of needed userspace headers above the sys/ ones (probably not all of these are needed).